Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify way of loading libraries in scripting addons #12662

Closed
NCC1701M opened this issue Feb 17, 2022 · 12 comments
Closed

Simplify way of loading libraries in scripting addons #12662

NCC1701M opened this issue Feb 17, 2022 · 12 comments
Labels
enhancement An enhancement or new feature for an existing add-on

Comments

@NCC1701M
Copy link

NCC1701M commented Feb 17, 2022

As described in this article the way of loading a library is

this.OPENHAB_CONF = (this.OPENHAB_CONF === undefined) ? java.lang.System.getProperty("openhab.conf") : this.OPENHAB_CONF;
load(OPENHAB_CONF + "/automation/lib/javascript/personal/utils.js")

The way a library is loaded could be simplified if the load function would handle relative paths.

For example:

If the load function is called like this

load("automation/lib/javascript/personal/utils.js");

the function could detect that a relative path was provided (on linux because of the missing / at the beginning of the string) and automatically prepend the OPENHAB_CONF path.

Otherwise if the function is called with an absolute path

load("/somewhere/on/the/filesystem/automation/lib/javascript/personal/utils.js");

it would not prepend the OPENHAB_CONF.

This would make loading libraries a little easier and would lead to better code because DRY (Don't Repeat Yourself).

@NCC1701M
Copy link
Author

No reaction on this?

@J-N-K
Copy link
Member

J-N-K commented Mar 13, 2022

IMO this is depending on the scripting language and needs to be addressed in the respective scripting add-on (if at all). I'm not sure if magically resolving paths is really a good idea.

@NCC1701M
Copy link
Author

There is no magic. It's a simple algorithm. The system should know best where the configuration folder is

@rkoshak
Copy link
Contributor

rkoshak commented Mar 14, 2022

As described in this article the way of loading a library is

As the author of that article I think it's worth chiming in here.

That article is for Nashorn ECMAScript 5.1 and only applicable to Nashorn ECMAScript 5.1. As far as I know, only Nashorn requires the use of the full path like that. Even the JS Scripting ECMAScript 2021 add-on just requires a simple const lib = require('name_of_library');. Jython too imports using relative paths.

Since Nashorn is going to go away as soon as we move off of JDK 14 anyway, I'm not sure it's worth spending effort on this at this time. But if effort were spent then I agree with @J-N-K, it's something that needs to be handled by the separate automation engines themselves (and in fact already is being handled that way already for all but Nashorn). I do think that Nashorn is handled by openhab-core and not as a separate add-on so this is where it would have to be implemented.

But that's assuming we can do anything about it. I'm also not certain that openHAB has much ability to control this. That load function and its requirements I think are defined by Nashorn itself. We'd have to create some sort of wrapper which means we'd need to include a helper library to abstract the openHAB stuff to Nashorn JS and implement that in that helper library (which leads to a catch-22 where you need to load a library to make loading libraries easier).

@NCC1701M
Copy link
Author

That article is for Nashorn ECMAScript 5.1 and only applicable to Nashorn ECMAScript 5.1. As far as I know, only Nashorn requires the use of the full path like that.

This was the only article I could found on how to use external scripts. The functionality has to be documented.

this is depending on the scripting language and needs to be addressed in the respective scripting add-on

There a a lot of GitHub Repos for openhab. For outsiders it's pretty difficult to tell where to place a request (in this case loading something sounds like a core funtionality).
There should be a repo or something similar to address such issues and those involved in the project should move them to the corresponding repo

@J-N-K
Copy link
Member

J-N-K commented Apr 29, 2022

As I said: this should be handled by the respective add-on.

@J-N-K J-N-K closed this as completed Apr 29, 2022
@NCC1701M
Copy link
Author

And as I said, for outsiders it is pretty difficult to determine which of those many repos is the correct one and that those people who are working on openhab should move those issues to the correct (AddOn-) repo.

So instead of only closing this issue, could you please open or move this issue to the correct repo?

Thanks

@J-N-K J-N-K reopened this Apr 29, 2022
@J-N-K J-N-K changed the title Simplify way of loading libraries Simplify way of loading libraries in scripting addons Apr 29, 2022
@J-N-K J-N-K transferred this issue from openhab/openhab-core Apr 29, 2022
@rkoshak
Copy link
Contributor

rkoshak commented Apr 29, 2022

As I said: this should be handled by the respective add-on.

But there isn't an add-on for ECMAScript 5.1. It's built into core as far as I can tell. I certainly can't find it here.

This issue is only a problem for ECMAScript 5.1 rules. The problem is already handled by all the other rules languages with add-ons here or the issue is irrelevant because the language doesn't support libraries in the first place (e.g. Rules DSL).

@J-N-K
Copy link
Member

J-N-K commented Apr 29, 2022

@rkoshak This requires at least writing a wrapper for the script engine. Since Nashorn was removed in Java 15, it'll not be available in core once we switch to Java 17 there. @wborn has created an add-on for supporting ECMA Script 5.1 (which is currently available from the marketplace). It makes much more sense to integrate it there instead of writing code which will be obsolete in the near future (especially with respect to limited core development capacities).

@rkoshak
Copy link
Contributor

rkoshak commented Apr 29, 2022

I completely agree so it makes more sense to me to open an issue on @wborn's repo which has the Nashorn add-on and close this issue here since it doesn't actually apply to any code hosted in this repo.

Or, were it me, I'd close it as "won't fix" given that Nashorn is on the chopping block and based on the marketplace entry, that add-on is mainly there to provide legacy support. I don't think there is an intention to continue development on Nashorn, or am I wrong?

@lolodomo lolodomo added the enhancement An enhancement or new feature for an existing add-on label Dec 12, 2022
@lsiepel
Copy link
Contributor

lsiepel commented Sep 22, 2024

See previous post. Raising an issue at wborn’s repo might be best. I would also not expect much development on this due to it being replaced.

@lsiepel lsiepel closed this as not planned Won't fix, can't repro, duplicate, stale Sep 22, 2024
@wborn
Copy link
Member

wborn commented Sep 22, 2024

Raising an issue at wborn’s repo might be best.

That's this repo because I contributed the code in #14013.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

No branches or pull requests

6 participants