-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[jsscripting] ES6+ Support #8516
Conversation
Travis tests were successfulHey @jpg0, |
Is this still being worked on? I can't review this if this PR stays a draft. |
@cpmeister yes it is, but it has a number of prereqs that need to go in first. These are the ones that are linked in the description. The intent is that once these are complete I can move this one out of 'WIP'. I can certainly push code against this PR so that you can see what it will be like, however it wouldn't compile and would be likely to change as the prereqs go in. (Also, I can certainty speed up opening more of the PRs in the description, however I have not been pushing too hard on this as the currently open ones have not been progressing very quickly. Any help on these would be appreciated!) |
Does this replace ES5 with ES6 or does it allow running them in parallel? |
@CrazyIvan359 great question. The way I have it working is that it replaces the script engine for any files ending in |
Great news! I don't think JS Transform needs to be a priority anyway. Such transforms are not the most common and are fairly simple so I don't think the limitations of ES5 are felt as much. |
This is pretty much ready for review, pending merge of the last remaining dependency. The surface area between this and the dependency are small, so I'll move this out of WIP now as it's pretty much ready for review. (It won't quite compile until that PR is merged, I'm expecting this.) |
Added minor change to remove hard dependency on script dependency tracking. Will now warn if this feature is not present, but otherwise will work (including compiling here). |
org.openhab.automation.jsscripting-3.0.0-SNAPSHOT.jar.zip Sample jar from my build. Untested, but should work on 3.0. |
I'm willing it to test it over the next week. Right now my setup is based on js files containing the rules with scripts around it. What do you recommend as a starting point? |
Maybe this would also be a good start for adding some content for the readme @LukasA83 and @jpg0. I have reduced the jsr223 generic documentation a bit for openHAB 3 since it makes more sence to me, to cover the actual state within those addons, to get the latest changes included just in time with the merge. If you can provide some starting point we can collect useful informations and file a pull request against your repository. :) |
Great idea! Some notes whilst I'm thinking about and in case anyone wants any hints...
My personal configuration is online: https://github.com/jpg0/oh-config/tree/master/automation so may be a useful reference. Should all the documentation be in the add-on repo? |
Yes. Jython docs are currently moving in the corresponding addon too and groovy docs will be served from this repo as well. |
@jpg0, I copied your org.openhab.automation.jsscripting-3.0.0-SNAPSHOT.jar into addons folder. And I have a started bundle Runs on Windows 10, installed is Java 11. But nothing happens. What am I missing for happiness? |
Nothing at all happens? Even without my add-on something should happen as the Nashorn engine should attempt to process the JS files (and fail). |
I'm able to load simple scripts, but the code is not compatible with openhab 3.0? I'm not able to load services etc. Here an example
win 10, graalvm, oh 3.0 release ! But you already said But I like you way you want to go, but is it not better to use the https://github.com/csowada/org.eclipse.smarthome.automation.module.script.jsext |
@csowada the error you are seeing there is triggered by the As for where the I'd be perfectly happy to see the structure change, or to support it, but I'm not a maintainer of that area or anything. |
As an OH user and an early adopter of @jpg0's great work here (the GraalVM integration bundle + his So first of all, I feel really excited about what's happening here. I think a solid integration of modern JavaScript could provide a huge boost for the future of OH, as...
Besides the modern language features of ES201x that we get here (the DX in Nashorn in comparison feels like browsing today's web with Netscape Navigator 😜), the ability to easily share that code is at least just as important IMO, building on top of the existing node module support (ESM hopefully in the future) and npm infrastructure. I always felt like there was some abstraction layer missing in OH, to build applications on top of the basic infrastructure (bindings etc.), that are more elaborate than just some simple if-then rules, are easily sharable and ideally can also be held accountable to some higher engineering principles (less spaghetti code wrapped in rules, better abstractions, modularization, test coverage etc.). With regards to sharing, it seems currently you have the choice of either reinventing the wheel, or looking through the forums to find some xtend based rule snippets with very ad-hoc style bespoke implementations, with zero separation between common logic and use-specific aspects (e.g. hard-coded item references all over the place). To give you some examples for applications (vs. simple rules) that I have on my to do list:
All of these examples have in common that they share common logic, are more elaborate that just a few simple if-then rules (e.g. could benefit from some higher level abstractions like using a finite state machine library like Tbh I am not really aware if those "rule templates" are a thing already and if they will support use cases like this (i.e. apps vs. rules)? But I do see a story of sharable code unfolding here with the support of modern JS and sharable npm packages (which the new GraalVM bundle supports, in contrast to the basic Nashorn engine AFAIK). Maybe in the distant future this could be made available in the UI, with some app repository (e.g. based on npm), generative forms for configuration and what not. But even in the most basic form as it would be with this PR, you could share code like this:
const alarm = require('awesome-oh-alarm-system');
alarm({
// specific config comes here, like listing all contact items, alarm signal devices, customized timeouts etc.
}); So, to support use cases like this, the work here is obviously super important. But based on my early experimentation with this, I see a couple of serious problems though:
Maybe not all of those concerns are relevant for the first iteration of integrating GraalVM, but I wanted to bring this up as early as possible nevertheless. At least it would be beneficial when adding things iteratively to not cause too much churn for users by introducing breaking changes (which mostly would be the case for the first point of library paths). That's it for now, sorry for the long rant! 😆 |
@simonihmig I think I agree with pretty much everything you have said! As another example to libraries - I actually have my HVAC implemented into a JS library just as you suggest, such that config drives when you want certain temperature ranges in different zones, plus which items control the zones, monitor the temp per zone, monitor the doors and windows in the zone etc. Regarding the problems you mention:
|
It seems to break the build, can you have a look at it @jpg0? |
I'm looking at it. I'm not sure what has changed (as it compiled before), but it looks like something related to nullability for type parameters. Compiles fine in my ide which is annoying... |
I am happy this is going forward now! 🎉 However what I am still missing is any decision or at least comment / note / thought from core folks regarding the issues that have been raised here, most importantly the issue of where libraries should live (in the current way in separate folders or in local And as @jpg0 stated, and I totally agree, the other "issues" can all be seen basically as missing features that can be added over time. But changing the module resolution later is a breaking change, and would be super annoying for early adopters of this new exciting JS integration! So at least something along the lines of "problem understood, our vision is this, next step is that" would be good to hear from the people who can make these calls! /cc @wborn @kaikreuzer |
Would it make sense for this add-on to also use any libraries in |
This is precisely how it works currently. The problem is that by default, node will always search Additionally, the current Possibly what could be done is:
This would likely provide compatibility with the existing lib split, and also allow users to switch to the "node native" approach. The only nasty part is having to build JS-specifics into |
What is with the idea of an |
Related to openhab#8516 Signed-off-by: Wouter Born <github@maindrain.net>
Related to #8516 Signed-off-by: Wouter Born <github@maindrain.net>
@wborn thanks for fixing up the build errors - was nighttime in Aus so couldn't get to it until later. |
@csowada The more I think about the use-cases related to this, the more I question whether we actually want that level of configurability. I see 3 main use-cases:
I'm not sure that we care about the first, as it will likely be handled by the UI, and generally does not require libs. The second use-case I personally would opt for the standard JS layout rather than a generic openHAB one (note that it would prevent mixing across language types - e.g. consuming a lib from a different language - but I'm not sure that we care about this). The third use-case prioritises the standard JS layout. This means that we should either optimise for one layout for JS (the JS/node one), or two (this plus the existing openHAB one). This feels much simpler than providing configuration allowing many. I'm sensitive to fragmentation of the user experience here, so prefer to have fewer options (as well as less code!). |
@jpg0 I have adjusted your PR a bit and it is missing just a little to implement more complex javascript. My change was the following. Customize ScriptFileWatcher
Here I see the ignore file to provide a common and simple approach for all script engines. Modify your PR
These two changes are enough to implement real node.js like projects. For this I chose a different directory than See the bit outdated changes here So two small changes would be enough for all openHAB users to be more flexible. And not too much code changes.
I would also see myself more as "Intermediate" or "Advanced" scripter as I use TypeScript for all of my rules :-) |
@csowada instead of adding a custom ScriptFileWatcher, would it not be better to allow the existing one to be configured by script add ons? So when the JS script add-on loads, it registers itself specifying both the script and lib paths? I'm concerned that a custom file (and hence user, rather than add-on configuration) will make things more complex. |
@jpg0 Oh, you are right. Wrong written. I mean that it makes more sense to modify the current ScriptFileWatcher, the custom one was only for my own test implementation. But you think it would be better to manage/modify the ScriptFileWatcher by the ScriptEngine. But this approach is more complex than a simple exclude file or not? |
I would expect an exclude file to be more complex. This is because in either approach, the ScriptFileWatcher needs to be able to accept and respect some kind of exclusion list. However with the file approach, it also needs to load and parse the file, monitor it for changes, apply changes during script loading etc. It's also something thing users would need to understand, rather than for it to 'just work' like node does. |
Signed-off-by: Jonathan Gilbert <jpg@trillica.com> Signed-off-by: John Marshall <john.marshall.au@gmail.com>
Related to openhab#8516 Signed-off-by: Wouter Born <github@maindrain.net> Signed-off-by: John Marshall <john.marshall.au@gmail.com>
Hi, I'm just becoming aware now that we have a new modern JS script engine, and I'm thrilled, that's great! From the UI perspective, could you confirm that once it's installed, it replaces the Nashorn engine for the I'm bringing that up because in the CodeMirror ecosystem that's used in the UI, you can choose which ECMAScript revision you want to target, and it will present you with different autocompletion hints. If there's no indication on the version of ECMAScript that's being used, the autocompletion might be inaccurate, and we don't want that. Also - I'm confident this new JS implementation can also work for Blockly script actions but we'll have to do thorough testing to confirm that. |
Currently, the implementation hides the Nashorn engine, but only in the scripted automation part of openHAB - not (for example) in the transforms part. As for how to tell whether it's installed, there is no explicit mechanism that I'm aware of to do this - I suspect that this may be the first example of two different engines implementing the same language type. One way would be to lean on OSGi and see if you can get a reference to one of the components this bundle exports. It does sound like it may be worth adding a feature into openHAB itself to make available the engine details for each script type. |
FYI to watchers of this PR, I have submitted another PR to have openHAB core ignore "./node_modules" for scripts, so that we can use this for JS libraries instead, as requested in this thread and elsewhere, to allow compatibility with 3rd party JS tools and libs. |
Signed-off-by: Jonathan Gilbert <jpg@trillica.com>
Related to openhab#8516 Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Jonathan Gilbert <jpg@trillica.com>
Related to openhab#8516 Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Jonathan Gilbert <jpg@trillica.com>
Related to openhab#8516 Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Jonathan Gilbert <jpg@trillica.com>
Related to openhab#8516 Signed-off-by: Wouter Born <github@maindrain.net>
Parent PR to track all the changes required to get ES6+ support into OH. This supercedes #6399 and was reopened from #8441
Remaining dependencies:
script module access ([automation] Extracted accessor into interface and added to script engine context … openhab-core#1843)engine identifier (blocks [automation] Added script filename to engine, if engineIdentifier is a file openhab-core#1196) [automation] Pass script context to script engines openhab-core#1837dependency tracking ([automation] Script dependency tracking openhab-core#1884)