openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
912 stars 420 forks source link

GraalVM JavaScript replaces Nashorn, breaking Blockly and existing Nashorn rules #2433

Closed rkoshak closed 2 years ago

rkoshak commented 3 years ago

Is there a way to have Nashorn and the GraalVM JavaScript add-on live side by side? Now it appears to completely replace Nashorn which breaks things.

I've had to deal with two problems in two days caused by the fact that GraalVM JavaScript is installed and then the users are trying to use a Nashorn rule or to use Blockly.

If it is not possible, I'll move this issue over to the webuis repo to have Blockly removed as an option to build rules when the add-on is installed. However, that's not a very satisfying approach. Installing an add-on shouldn't replace or break stuff that is built into openHAB be default in my opinion.

openhab-bot commented 2 years ago

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/extending-blockly-with-new-openhab-commands/127169/20

digitaldan commented 2 years ago

Interesting that

const logger = Java.type("org.slf4j.LoggerFactory");

Loads fine (and i can use to log), but

const MessageFormatter = Java.type("org.slf4j.helpers.MessageFormatter");

Throws the error i mentioned before. It certainly feels like it's not in the context class path, but looking through the binding I would think it would be available (you specifically import org.slf4j). I have never spent any time understanding OSGI/BND class loading, so it may be clearer to someone else.

jpg0 commented 2 years ago

I am wary of the slf4j case because at one point I tried to use the slf4j message formatter directly and couldn't do so. I investigated quite deeply into the logging framework however it gets very complex because not only is slf4j a framework for plugging in loggers, but it's actually provided by pax, which itself is a framework for plugging in logging frameworks / apis. Add to this the fact that their are multiple versions of slf4j around, and they are all handled by OSGi. I got as far as determining that there were class instance clashes somewhere deep down in log4j (plugged into slf4j) which resulted in consumers not being able to get a message formatter. Also note that the way OSGi deals with packages is that they do not cascade/nest - so even if you have access to org.slf4j you may not have access to org.slf4j.helpers.

I guess this is just a very long way of saying that I don't regard not being able to load the slf4j message formatter as necessarily pointing at a problem with the JS plugin! However, https://github.com/openhab/openhab-addons/issues/11222 does have another example which does point at the plugin.

digitaldan commented 2 years ago

Got it, understood, i spent some time trying to debug it, and it is indeed very complex (no surprise there). After removing the call to MessageFormatter and bypassing formatting, logging did work, but now i have the same problem as https://github.com/openhab/openhab-addons/issues/11222 where i am not able to access OH classes

org.graalvm.polyglot.PolyglotException: TypeError: Access to host class org.openhab.core.items.Metadata is not allowed or does not exist.

And stopped there last night :-)

rkoshak commented 2 years ago

Just to add a little bit on logging. I would expect that we would want to prioritize using the openHAB log actions over slf4j anyway. So maybe it's best not to spend too much time on that. Not being able to access anything in org.openhab.core is going to be a problem though.

digitaldan commented 2 years ago

So i have been playing around with this a bit, and am baffled why the graalvm context is seemingly not able to load these classes from it's default class loader (or a number of different class loaders i tried manually feeding it). Not sure if anyone else have been looking at this or has ideas?

openhab-bot commented 2 years ago

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/simple-tutorial-ecmascript-doesnt-work-java-lang-nullpointerexception/127498/7

openhab-bot commented 2 years ago

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/extending-blockly-with-new-openhab-commands/127169/32

digitaldan commented 2 years ago

So I am not able to get this work, i'm sure this is a OSGI/bnd classloader issue, but i can't find a solution to get it to load our org.openhab.* packages. @jpg0 any ideas ?

digitaldan commented 2 years ago

🤦 So i got it working.... i would of had this working on Friday (or earlier), but i just realized i had an error in my build command that was not loading the jar correctly in OH, so i wasted days of testing . I'll submit a PR for this shortly after i test some more 🤦 🤦 🤦

digitaldan commented 2 years ago

https://github.com/openhab/openhab-addons/pull/11400

rkoshak commented 2 years ago

Just in case I've lost the ball here. As soon as #11400 gets merged the work around @ghys posted above to make a rule compatible with both Nashorn and GraalVM JS should work, correct?

I'm keen to try porting my rules and learning so we can start building some docs, or at least some conversion tutorials and the like.

digitaldan commented 2 years ago

As soon as #11400 gets merged the work around @ghys posted above to make a rule compatible with both Nashorn and GraalVM JS should work, correct?

I would think so, yes. Java.type() now works and uses the correct classloader. @jpg0 's ohj library also now runs without modification.

digitaldan commented 2 years ago

Hey @jpg0 i have a question about some imports in ohj, i see scoped imports like @runtime/Defaults, @runtime/provider @runtime/RuleSupport and @runtime/osgi where are these libraries provided ?

jpg0 commented 2 years ago

@digitaldan these are looked up at runtime. Any bundle can provide any object (such as itemRegistry) into any namespace (such as RuleSupport). There is also a 'default' namespace which is automatically included for other runtimes, and can be imported just as @runtime in JS.

Look at https://github.com/openhab/openhab-core/blob/main/bundles/org.openhab.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/defaultscope/DefaultScriptScopeProvider.java to see an example (this provides all/most of the default things).

digitaldan commented 2 years ago

Ahh, thank you! i guessed it was provided but was having a hard time finding where.

digitaldan commented 2 years ago

So i have a very raw, proof of concept that merges the OHJ library into the jsscripting binding.

It's not pretty......but i was curious :-)

First, the really ugly..... I wanted to load the included ohj library along with user provided libraries in ${openhabdir}/conf/automation/lib/javascript/personal. This was a bit difficult, as the script context only lets you supply one path.

So the ugly hack here is to copy a resource from the bundle to a tmp directory and "side" load it using the Filesystem provider in the binding. I originally tried to load it directly from the bundle classpath through this provider, but could not make that work (hence the ugly hack of copying to a tmp dir).

There is also an option to supply a global script object , but i'm not sure it supports require imports in that script, or at least i was not able to figure that out (which could be related to it being in the global engine scope?) .

Hopefully someone who knows what they are doing here can help :-)

Some good things, i'm using webpack to build the library as a single oh.js file, which is very small and gets copied into the jar at build time. All of this is done from the pom.xml file (with bits borrowed from our UI pom), so the binding contains everything it needs to build and deploy.

The library is now oh instead of ohj so code would look like:

const log = require('oh').log("test");
log.info("Hello World");

const { items } = require('oh');
items.getItem("F1_Office_Light").sendCommand("ON");

see https://github.com/digitaldan/openhab-addons/tree/jsscripting-ohj/bundles/org.openhab.automation.jsscripting

jpg0 commented 2 years ago

@digitaldan regarding the dual paths, my advice would be to intercept the require loading rather than operate at the filesystem level. I already do this to supply the built-ins via the @runtime import. Possibly you could intercept @oh and then direct this to the resource in the bundle? (Personally I prefer @oh as using the @ sign can disambiguate against real imports - in this case https://www.npmjs.com/package/oh)

I am hoping to be able to help out soon, especially as I need to move the library directory itself (away from /lib/javascript/personal) which overlaps with your changes, just some other things that I need to address before I can start...

digitaldan commented 2 years ago

my advice would be to intercept the require loading rather than operate at the filesystem level. I already do this to supply the built-ins via the @runtime import. Possibly you could intercept @oh and then direct this to the resource in the bundle?

I agree this would be a better way, trying to hack the filesystem is, well, a hack. I did look at the ScriptExtensionModuleProvider to inject as you mentioned, but realized i had no idea what I was doing :-) Its going to take me a bit to understand the scripting backends, its quite complex.

Personally I prefer @oh as using the @

That sounds perfectly reasonable

I am hoping to be able to help out soon

That would be awesome, this little experiment made me realized how little I understand about our openhab-core scripting framework and the GraalVM/scriptManager ecosystem, and I'm probably not going to be doing anything the right way :-)

digitaldan commented 2 years ago

FYI, i did update my POC to at least load the ohj library directly, and not rely on hacking it out to the real filesystem, this actually works pretty nice for testing, if you have the oh js library in your personal path, it will use this and not load our internal one. I also changed it to use @oh when importing.

digitaldan commented 2 years ago

Here's a jar if anyone want to try it out, imports work like: const { items } = require('@oh');

https://github.com/digitaldan/openhab-addons/releases/download/jsscripting-OHJ/org.openhab.automation.jsscripting-3.2.0-SNAPSHOT.jar

digitaldan commented 2 years ago

I have been attempting to eat our own dog food here and start porting all my many production DSL rules over to jsscripting/ohj. During this process i have been making several changes to the library, mostly adding new triggers and fluent wrappers for them, as well as some smaller stuff. I would also like to start removing some of the legacy support for ESH and OH 2 from it as I don't think its needed.

It would be nice if we could submit an actual WIP PR to the JSScripting binding with the code from the OHJ 3.x branch so we can start centralizing these changes. @jpg0 since you have done all the awesome work to create OHJ, is that something you would like to do ? Or is there another way you want to start collaborating on it? We can also then move the conversation over to the PR and close this one.

I would be happy also then to submit my change to the binding to load the OHJ library, even if we end up changing how that works later as it at least gives us a fully contained preview now of what i think we are looking to release.

digitaldan commented 2 years ago

Also I was thinking we might want to continue to keep @oh/ohj as a separate repo and keep publishing as an NPM. This would make development easier for others, allow local resolution of the library in IDEs, etc... Thoughts ?

simonihmig commented 2 years ago

To add some thoughts here...

  1. Regarding explicit imports, I am strongly in favor, no matter where. Just to reiterate: they are more or less required for "pro users" when you want to use tools like ESLint or TypeScript and not totally confuse those by having "magically" injected things in the global scope. Also I believe they help for learning, tracing and understanding. When I want to see what a thing does, even without documentation, I can lookup the imported package on npmjs.org, find its github repo and read the source. When it is just there somehow, I have no idea where it comes from.

  2. regarding the module system itself, when we are now talking about requiring users to get themselves used to having imports in their (UI or file-based) scripts, eventually for the first time unless they are already experienced JS devs, then I think we should teach them to use the module system that is already in large parts the dominant force in the JS ecosystem, and likely IMO to fully take over in the future. And this is not CommonJS (CJS) (require()), but the language's official ECMAScript modules (aka ES modules or ESM), so import ... from .... As I said, it is already the standard in frontend/browser world (meanwhile supported natively in browsers, but usually compiled away by bundlers), and AFAIK has growing usage in the node.js world, with node 14 having added full official and stable support.

    For readers not having followed the JS ecosystem closely: this has been some "battle" of the last years of CJS vs. ESM. Node had to introduce its own module system (CJS) long before ESM was a thing. But with the emergence and dominance of ESM, supporting both in node was apparently not trivial due to different semantics (require() is a run-time call, while import is strictly static. To add to the confusion, ESM's import() is also a run-time call, but async)

GraalVM seems to support ESM already, but I don't know if that already works, or what needs to be done to make it work? Maybe @jpg0 knows?

  1. Do you really think that bundling the (JS) helper library with the (Java) addon is the right thing? Unless I am misunderstanding this, this does mean that you will get a new version of the library only when updating the addon, i.e. with a new OpenHab release? On the other hand, there is already a de-facto standard way to distribute JS, and this is npm. So for file-baed scripts, that would be the first choice anyway, to add this and possibly other dependencies to your package.json and run npm install (when the resolution system has been changed to look for libraries with the node module resolution algorithm, i.e. in the local node_modules, but this is planned when I understand the previous comment correctly). But maybe something similar could work also for UI based scripts, by adding the possibility in the UI to declare the script's dependencies in the UI as well, similar to what online coding platforms like codesandbox do. It could be simplified to just create a list of dependencies (with their version), which under the hood gets transformed to a package.json and an npm install call when saving? The helper library could be added by default (same as its import), but at the end it is just a regular dependencies like any other. So you could also add other dependencies (the example of chroma.js has been given before). And eventually the editor could be replaced with for example the Monaco Editor, to offer IntelliSense like capabilities (especially when dependencies provide TypeScript typings, so you get full autocompletion for 3rd party APIs etc.). I know this is a longer road, but maybe setting the course has to be done now, by going all-in on an npm-based distribution mechanism?
digitaldan commented 2 years ago

Thanks for the insightful comments, I agree with 1), and for 2) I'm not an expert in CJS or ESM , my only opinion would be to prefer open standards and try and future proof (as much as we can) our implementations.

For 3) i think it's important to ship with a default version of the library, so stuff "just works" out of the box, especially users that only use the UI. We also support offline installations, so i think it's important to be compliant there. That being said, I think we should support loading the library first from the users personal path, and fall back to the shipped version. I think this is the best of both worlds, and in fact how i have been locally developing against my production OH instance (and how my version of the binding is working ).

rkoshak commented 2 years ago

For 1 I'm ambivalent. I agree that we need to enable the "pro" users. But to me it's really hard to justify, especially in UI rules, when you end up with 50% or more of the entire code you have to write being import. Remember in the UI you can't just put the import at the top of a file and reuse them for a dozen rules. No, you have to import them again and again for each and every Script Action and Script Condition. And since a given rule may have more than one of each of these you could have to reproduce the same import lines four or more times for a single rule.

It's a ton of boilerplate that gets imposed on the users who we need to help the most. It seems like pretty big imposition to me.

And these users are not going to be looking up the source code or running linters or anything like that. They just want to write a dozen or so lines of code so their light turns on at the right time. Based on what I've see on the forum, the average length of a rule (not counting the proforma and triggers) is about 10 lines of "functional" code. Even requiring the addition of two import lines is a 20% increase.

However, having said that, there are "pro users" who I would hope are writing rules in the UI, but that is mostly for the creation of rule templates. So we absolutely do need to support them too. I don't want to cut them off either. But frankly, those users are best able to handle a little extra complexity to make the more standard approach with no imports work. I'd much rather force the experts to go through a couple extra hoops than force our average users to do so instead.

I've no opinion on 2. What ever makes sense is fine.

For 3, yes I do. Frankly, without the helper library associated with an automation add-on it's a pretty miserable experience and almost completely out of reach for most of our users.

I'm very much against only relying on npm or gems or pypy to distribute the helper libraries. Why impose on our users this extra step? Remember, most of them are not developers. Do we really want to tell them "install the GrallVM JS addon. Then install NPM on your machine (here's the Windows instructions, here's the Linux instructions, here's the Mac instructions) . Now install the helper library. OK, now you can write your first hello world rule since we now have access to the logger in a sane way."

These libraries are only useful in an openHAB context. They are meaningless everywhere else. And rules development is almost completely out of reach on openHAB without them. So why force our users to go get them from somewhere else?

Forcing every UI Script Action and Script Condition to have to import the same thing(s) every single time and forcing these users to have to install a third party package manager just to get the library that lets them interact with openHAB in a human friendly way seems like a pretty big burden to place on our least capable users.

digitaldan commented 2 years ago

It's a ton of boilerplate that gets imposed on the users who we need to help the most. It seems like pretty big imposition to me.

i agree with this, I would love to find a way to support "out of the box" behavior that "just works" for most people, and give power users flexibility to start with a clean slate.

For example, after only a few days of moving my rules over, i have had to write my own wrapper to reduce the amount of boiler plate code i'm using. Below is my "super" namespace that include oh and runtime, so i'm not having to specify this in every single file.

I also added a proxy to dynamically gets items when called as a member of items, for example items.F1_Bedroom is equivalent to items.getItem("F1_Bedroom") (which becomes tiresome to see over and over in a rule. )

const ns = {};

// assign everything in @oh, so items, actions, triggers, etc....
Object.assign(ns, require("@oh"));

//import everything in @runtime to a runtime var 
ns.runtime = require('@runtime');

//while native Date objects do work, ZoneDateTime is much more convenient  
ns.ZonedDateTime = Java.type('java.time.ZonedDateTime');

//alow dynamic getItem calls like items.F1_Master_Bedroom, but still support getItems and other default functions
ns.items = new Proxy(ns.items, {
    get: function (target, name) {
        return target[name] || items.getItem(name);
    }
});

module.exports = ns;

I'm then importing the exports of this super namespace into the local scope in every rule file, which looks like:

Object.assign(this, require('@ohc'));
const log = require("@ohc").log("foo");

I would really love to minimize the amount of this type of code for our default users, most who will not be javascript experts, and my feeling is a majority just want sane defaults. I think we can make the binding a configurable-service so users could change the behavior to meet their specific needs, I was thinking a Boolean option to inject or not, and a String value that contains the injection code (like Object.assign(this, require('@oh'));)

image

jpg0 commented 2 years ago

@simonihmig when I originally coded this add-on, GraalJS did not support ESM, and I have not tried it since.

I do not believe that importing host runtime (@runtime) things would work, as that is currently fairly "custom" (hopefully won't always be, but there is no clean way to intercept/hook into module loading), but I see no reason why it couldn't be made to work.

jpg0 commented 2 years ago

One comment that I want to leave on explicit vs. implicit imports: it's not about putting the complexity on beginner vs. pro users - yes the beginners inherit boilerplate (would could be called complexity), but pros get incompatibility (which is more akin to impossibility).

I think something that frustrates me here is that we don't seem to have a view on what sort of scripting/rules beginners should be doing. We are discussing here whether JS should be brought down to a level of simplicity where it is essentially no longer standard JS and loses compatibility with the wider JS ecosystem. Should we also be doing this with the other scripting languages like Python and Ruby? And even if we did, what should beginners be using in this case, given that there are multiple languages? Isn't even providing this choice for non-devs adding complexity?

My belief is that we should strive to offer great support for these scripting languages so that they can be used as the language is intended, then build a preferred beginner experience on top of them (most likely choosing a single language, or maybe even Blockly abstracts the language away).

digitaldan commented 2 years ago

Isn't even providing this choice for non-devs adding complexity?

So i am proposing we default the choice for non developers, which is much of our user base, and allow power users to change it. Or at least default it for UI based rules . I guess i'm failing to understand why this does not satisfy all parties?

We are discussing here whether JS should be brought down to a level of simplicity where it is essentially no longer standard JS and loses compatibility with the wider JS ecosystem.

Apologies, but I'm not understanding this, so by optionally adding an import to scripts so most users (or most UI users) do not have to do this every time (especially in small UI based scripts) looses compatibility with the JS ecosystem? I'm feeling like i'm still missing something.

My point of view is that for the vast majority of users, this is strictly a scripting environment for light weight procedural logic, most will not import other libraries, manage NPM packages , etc... But we can still provide an environment for those who want a full blown Node runtime and all the power that comes with it.

I think something that frustrates me here is that we don't seem to have a view on what sort of scripting/rules beginners should be doing.

My intention is not to frustrate anyone, but having been around the OH community for a long time, an out of the box scripting environment like this based on the language of the internet is huge for openHAB, i think @rkoshak probably knows better then everyone here who our user base is and what is going to resonate better, so I take his opinion very seriously here.

So to be clear, you would like to still see OHJ built into the jsscripting binding, but not have an option to implicitly import its members (items, actions, log, etc...) and always require the user to do this?

jpg0 commented 2 years ago

So i am proposing we default the choice for non developers, which is much of our user base, and allow power users to change it. Or at least default it for UI based rules . I guess i'm failing to understand why this does not satisfy all parties?

If it's possible for power users to change it, without meaning it causes some incompatibility, this is possibly fine. But that's not my point. The discussion here appears to be about whether or not we dilute idiomatic JS into something simpler for non-devs. But I don't believe that this is the right discussion to have. I believe that we should be specific about how non-devs are targeted. Possibly they should not be using JS at all, but they should all be directed to Python (if they are not devs, then why try to support them with more than a single language?). Possibly the experience should be entirely visual, via Blockly or similar.

Without doing this, I fear that we end up with a situation that is not ideal for devs or non-devs, because we haven't defined these personas and explicitly built things to support them. (This is aside from the fact that almost all solutions like this involve building in layers of abstraction - where the simpler, less capable option (for non-devs) would build upon the lower level on (for devs), with both being available in some capacity.)

looses compatibility with the JS ecosystem

Yes, if there are symbols dynamically injected into the runtime then bad things may happen, such as:

an out of the box scripting environment like this based on the language of the internet is huge for openHAB

Do you believe that JS should be the primary language that we encourage for non-devs? I am not against this at all, but I do not believe that this decision has been made?

So to be clear, you would like to still see OHJ built into the jsscripting binding, but not have an option to implicitly import its members (items, actions, log, etc...) and always require the user to do this?

digitaldan commented 2 years ago

But I don't believe that this is the right discussion to have. I believe that we should be specific about how non-devs are targeted. Possibly they should not be using JS at all, but they should all be directed to Python (if they are not devs, then why try to support them with more than a single language?).

Thats fair, and i would agree not to mix up the larger discussion on how should non devs write rules with this one, which should be about getting OHJ into the binding. I have my own opinions on this, but i also think its more important to get a useable JS rule engine out.

I'm fine with it being provided by the binding, but with the caveat that I think that it should also exist on npm, be updatable via npm

I agree this should be a npm library and also its own repo so it can be updated independently of the binding. Right now the binding will use the first one in the node search path, so if you do a npm install of @oh in your automation/lib/javascript/personal/node_modules directory (or drop a webpack @oh.js single file there), this will be used over the shipped one, is this sufficient for being able to upgrade? This would also allow for an "explicit" version of the library if a user cares to upgrade.

I would even be happy with a hardcoded "import all the things" header,

So for a blank UI rule, something like this will be inserted into the top (and the user could remove/change it if necessary)?

const {items, things, metadata, triggers, actions, utils, osgi, provider}  = require('@oh');
const log = require("@oh").log("JS Rule");
jpg0 commented 2 years ago

is this sufficient for being able to upgrade? This would also allow for an "explicit" version of the library if a user cares to upgrade.

I believe so, yes.

So for a blank UI rule, something like this will be inserted into the top (and the user could remove/change it if necessary)?

Yes. I'd be happy with either an explicit list of symbols like that, or even Object.assign(this, require('@oh')); to get everything. Either inserted into the raw code, or simply displayed at the top. Could even be hidden in a collapsible section. I do think that copy & paste should capture it though, so that everything is explicit if pasted in forums etc.

rkoshak commented 2 years ago

Without doing this, I fear that we end up with a situation that is not ideal for devs or non-devs, because we haven't defined these personas and explicitly built things to support them. (This is aside from the fact that almost all solutions like this involve building in layers of abstraction - where the simpler, less capable option (for non-devs) would build upon the lower level on (for devs), with both being available in some capacity.)

Frankly, this is why I've been so vocal and yet also so demoralized over this whole topic for the past couple of years. Everyone who is developing on languages for rules (except for Blockly) are targeting and fighting to preserve some pure idomatic environment. And that's fine. But for most of our users that either puts that language completely out of reach or makes for a pretty poor experience when all they want to do is some light scripting of their automation.

I thought that this whole thread was to make GraalVM JavaScript be that language that targets the new users. I guess not? If not I'm not sure what it would be. Rules DSL lacks some features that make it unsuitable plus is anyone actually willing to keep it up? Nashorn and Jython are non-starters since they are basically deprecated. Blockly can be part of the solution but I don't think it can be the only solution. There is no other automation add-on that I'm aware of that has even been submitted for consideration for inclusion into OH. So it's GraalVM JS or nothing.

I thought that @digitaldan's approach of giving the option for the developer among us to have a pure environment through a flag/property and let the rest of the users have a "dumbed down" environment where they just need to focus on what they need to do and not actually learn how to program idomatically was inspired and a great compromise. We all get what we want. I don't see what that's a problem except that there is some sort of rejection of the mere existence of a "dumbed down" option.

Possibly they should not be using JS at all, but they should all be directed to Python (if they are not devs, then why try to support them with more than a single language?). Possibly the experience should be entirely visual, via Blockly or similar.

But the Python devs want to have pure idomatic Python. The jRuby devs want to have pure idomatc Ruby (and I can't tell if they even have an interest in making jRuby be a part of OH or plan on continuing to maintain it as something deparate). Everyone want's their pure environment.

And there is no Python support in OH that has any sot of future. Jython is still 2.7 with no 3.x version in sight. And the 2.7 version is barely supported. Until someone creates a GraalVM Python add-on, it's GraalVM JS or nothing. And whose to say that the developers of Python won't equally reject to a "dumbed down" version for rules development.

Who wants to be the default language to support new and non-technical users? Not I says GraalVM JS. Not I says jRuby. Not says Java rules. Not I says scripted Java.

Focusing on Blockly can help but frankly it won't take long before most users get tired of it and want something text based. It's also not super fun to write rule templates in. And there while we are about to get a big drop of new functionality, it's still not complete. In short, there is a huge gap between Blockly and idomatic JavaScript that I think needs to be addressed.

Yes, if there are symbols dynamically injected into the runtime then bad things may happen, such as:

But the whole point of the boolean parameter is, if these are going to be a problem the symbols can not be injected. Am I wrong? If I flip the switch for a rule so nothing is injected there should be conflicts.

And it's worth mentioning that only a very small minority of OH users will ever encounter any of these problems.

Do you believe that JS should be the primary language that we encourage for non-devs? I am not against this at all, but I do not believe that this decision has been made?

I thought it had and the answer was yes. As of right now it's the only choice.

I'm fine with it being provided by the binding, but with the caveat that I think that it should also exist on npm,

That I'm fine with.

then telling users to leave the UI if they wanted to use a library (well, that's assuming that we don't care about library support in the UI...)

I'm not completely sure about that. What about what we currently call "personal" libraries for Nashorn JS? Those won't have symbol conflicts because they have been written with all those symbols in place in the first place. I wouldn't want to eliminate that possibility. If we did, we may as well fall back to Rules DSL. I don't want to eliminate someone from being able to, for example write a library function to centralize their alerting code, import that and call that from a Script Action/Script Condition.

So for a blank UI rule, something like this will be inserted into the top (and the user could remove/change it if necessary)?

I don't care if it's visible or not. I like the idea of being able to edit it if desired. I just don't want the users to have to type it nor do I want the users have to look stuff up to do common and core interactions with OH like sending a command or creating a Timer. Almost everything one does in a rule is interact with OH. That interaction should be as seamless as possible.

I do think that copy & paste should capture it though, so that everything is explicit if pasted in forums etc.

I am pushing people away from posting rules to the forum and towards creating rule templates instead. We need to get away from forcing all users needing to copy/paste/edit everything and towards users installing and configuring rules. Having said that, there too the imports need to be included in the YAML/JSON representation of the code too.

Having written a few I can say that it's easier to support and easier to use to have templates instead of copy/paste/edit code examples. So I do have this use case strongly in mind in this whole discussion.

lewie commented 2 years ago
2. Clone `https://github.com/jpg0/ohj.git` into `$OPENHAB_CONF/automation/lib/javascript/personal/node_modules/`  and run `npm install` inside `ohj`

3. Alternatively install `ohj` using `npm install ohj` from  inside `$OPENHAB_CONF/automation/lib/javascript/personal/`  (i tried both 2. and 3.)

4. Load a test script:

@digitaldan, can you please help me out a bit, I am unsure. The directory structure below $OPENHAB_CONF/automation/, who or what creates this. Or can I just create the above manually? Or am I missing something?

digitaldan commented 2 years ago
  1. Clone https://github.com/jpg0/ohj.git into $OPENHAB_CONF/automation/lib/javascript/personal/node_modules/ and run npm install inside ohj

This is the correct path, so when doing a require('ohj') graalvm will check $OPENHAB_CONF/automation/lib/javascript/personal/node_modules/ohj, make sure you are using a nightly build which contains the correct classpath fix.

digitaldan commented 2 years ago

So i have a version of the binding that optionally injects a code block. I also have the binding now as a configurable service to make this optional

image

I think this is simple to understand and consistant (although the exact wording and default injection code syntax is up for debate) . We could also have the options include a 3rd choice which is to only inject on UI based rules, although short of having the UI read this config and do the injection itself, i'm not sure how i would know in the scripting engine where the script to evaluate is coming from (UI or File)

rkoshak commented 2 years ago

This looks simple enough and clear. It's not too clear from the screen shot, the Injection code can be edited?

Does the code appear in the Script Actions/Script Conditions as you are editing them or are they wholly handled by the setting?

Is this setting global or configured on an action by action basis?

I'm thinking that maybe we don't inject in file based rules at all. Because you can define more than one rule per .js file the burden of needing to import stuff is lessened. So maybe having the UI do it is the way to go.

digitaldan commented 2 years ago

It's not too clear from the screen shot, the Injection code can be edited?

Yes, I though it would be good to make it clear what is being provided, and give users the chance to customize ( if you clear it, it will revert back to the default value) .

Does the code appear in the Script Actions/Script Conditions as you are editing them or are they wholly handled by the setting?

All this does is intercept the eval(String string) call to the script engine and prefixes the string with the code. The UI could read the configuration from /rest/services/org.openhab.automation.jsscripting/config and decide if it needs to display this injected code (so the user sees what is being injected, but would not be able to edit them).

I'm thinking that maybe we don't inject in file based rules at all.

I could go either way. I was thinking it might be nice to give three options. The first to always inject, seconds is to only inject in the UI (so the binding actually never injects, and the UI would need to read the config from above rest endpoint and add to every script on creation), and third is never inject. I think option number 1 is actually quite useful for people who are still using file based DSL rules which have these implicit variables already there, so it's a natural transition pattern. Option 2 is probably ideal for most use-cases as using the UI without implicit variables is already very painfully verbose. And option 3 is going to be useful for those who want absolute control over everything which i am happy we can support.

digitaldan commented 2 years ago

@jpg0 I would love to get the ball rolling to move OHJ to the openHAB git repo and make it an official NPM module, i can create a repo for it if you would like to do the initial push? I have a number of additions (triggers and such) i would like to get your feedback on. I figured getting the 3.x branch over as a starting place is a good first step. Any thoughts on the name?

jpg0 commented 2 years ago

@digitaldan do you mean a repo under the openHAB organisation? Sure, I'd be happy to push the first version there.

As for the name, I did struggle a little bit with this when I first started it! I originally appended the 'j' to signify that it was javascript, but I'm not sure that is really necessary as the context provides this (well, the module name, not the repo name). Do you know if any of the other languages use a specific name for helper things that get imported?

digitaldan commented 2 years ago

do you mean a repo under the openHAB organisation? Sure, I'd be happy to push the first version there.

great! I can create this and give you access..... as soon as we decide on a name :-)

Do you know if any of the other languages use a specific name for helper things that get imported?

we typically prefix official projects with openhab-foo , I think openhab-js sounds great, so lets start with that, I'll add that now, so look for a ping from github when i add you too it. Very excited!

digitaldan commented 2 years ago

fyi https://github.com/openhab/openhab-js is ready for some code.....

jpg0 commented 2 years ago

Just tried to push, but I don't think I have access to the repo...

digitaldan commented 2 years ago

Github sent you an invite you need to accept, not sure if this is email or something else?

image

jpg0 commented 2 years ago

Ah, that was in my GH notifications. Weird that you need to explicitly accept an access grant to a repo... but I accepted and pushed the code, so it's there now.

wborn commented 2 years ago

Weird that you need to explicitly accept an access grant to a repo

You might find it less weird if random strangers start adding you to repos you don't want anything to do with. :wink:

digitaldan commented 2 years ago

Awesome @jpg0, thanks!

jpg0 commented 2 years ago

@wborn that's true if access control is conflated with other things (like notifications etc), but otherwise why should I care if I have access to someone's repo? It's the same as if someone makes a writable-to-the-world repo (which implicitly includes me); I wouldn't care. Unfortunately many cloud systems mix up access control with other concepts.

openhab-bot commented 2 years ago

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/usage-of-new-ecma2021-automation-scripting-graalvm/122724/29