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

[JSR223] Mention difference between NashornJS and GraalJS #1846

Merged
merged 7 commits into from
Jul 12, 2022

Conversation

florian-h05
Copy link
Contributor

Fixes openhab/openhab-js#139.

This explains the difference between the two available JavaScript options and recommends to use JS Scripting.

Signed-off-by: Florian Hotze [email protected]

@netlify
Copy link

netlify bot commented Jun 27, 2022

Thanks for your pull request to the openHAB documentation! The result can be previewed at the URL below (this comment and the preview will be updated if you add more commits).

Name Link
🔨 Latest commit f0f86af
🔍 Latest deploy log https://app.netlify.com/sites/openhab-docs-preview/deploys/62c58f4e7314a90008d42715
😎 Deploy Preview https://deploy-preview-1846--openhab-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@rkoshak rkoshak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this. I added a few comments based on the current state of Nashorn.

Over all, I think our whole approach to the rules documentation needs an overhaul but I think we should wait to see if Rules DSL gets spun out into a separately installable add-on.

configuration/jsr223.md Outdated Show resolved Hide resolved
configuration/jsr223.md Outdated Show resolved Hide resolved
configuration/jsr223.md Outdated Show resolved Hide resolved
@florian-h05
Copy link
Contributor Author

@rkoshak
This is now ready for re-review, I also added an example rule for JS Scripting.

@rkoshak
Copy link
Contributor

rkoshak commented Jun 30, 2022

The text looks great!

I'm ambivalent about the JS Scripting example.

On the one hand it's useful and clearly illustrates the differences between them. And there are examples for all the other languages there already (except Rules DSL for obvious reasons).

On the other hand, JS Scripting isn't actually JSR223 so there is a strong argument that an example like this doesn't even belong here, especially since just about everything that is discussed on this page is hidden behind the JS Scripting Helper Library which is documented elsewhere. Were the example to only use the JSR223 interface as it's discussed on this page, it would look almost exactly the same as the Nashorn example with the addition of a require("@runtime");.

Given that the topic of this page is the JSR223 runtime interface, any JS Scripting example should probably use that interface directly, perhaps with a note that using the Helper Library results in a much simpler and straight forward implementation (which is also true for jRuby which probably should have a similar note ).

@florian-h05
Copy link
Contributor Author

Reagrding JS Scripting:
What about adding a note to the code example that this example is using the official helper library to demonstrate it's functionality?

@rkoshak
Copy link
Contributor

rkoshak commented Jul 1, 2022

That would be a minimum but I'm still a little concerned. jRuby could do the same thing. But still one has to ask what's the point of the example if it's not demonstrating the APIs that the page is documenting? This page documents the Java API but the helper library almost complete wraps/replaces that API. And the Helper Library is fully documented elsewhere so if I wanted to look on this page for more information about JSRule, triggers, etc. I wouldn't find any of that here on this page.

It's confusing to have an example that doesn't actually demonstrate the use of anything that's documented on the page is my main point I think. I can think of ways to make that make sense but it'll require almost a complete restructuring of this and the Rules documentation pages or the introduction of another "generic rules concepts" page.

@florian-h05
Copy link
Contributor Author

florian-h05 commented Jul 2, 2022

@rkoshak

I updated the JS Scripting example to have one using the raw Java APIs and to have the same using the helper library (with a comment that clearly indicates that it is using the helper library).

However, I was not able to get the example for the raw APIs working with @runtime, I instead used Java.

Just look at the docs preview.

@Confectrician Confectrician added the Feedback Waiting for feedback label Jul 3, 2022
Copy link
Contributor

@Confectrician Confectrician left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from docs side.
Please give me a ping, when it is ready for review/merge from the content side too.

@rkoshak
Copy link
Contributor

rkoshak commented Jul 5, 2022

So this didn't work? Or did you try to import some other way?

I've not done file based rules with JS (either version) so I don't know if there is something different. But when I was looking in how to make Nashorn compatible with JS Scripting, the easiest thing was to pull in @runtime and then any call to the OH API classes would go through the runtime variable.

var runtime = require("@runtime");

runtime.scriptExtension.importPreset("RuleSupport");
runtime.scriptExtension.importPreset("RuleSimple");

var sRule = new runtime.SimpleRule() {
    execute: function( module, input) {
        print("This is a 'hello world!' from a JavaScript rule.");
    }
};

sRule.setTriggers([
    runtime.TriggerBuilder.create()
        .withId("aTimerTrigger")
        .withTypeUID("timer.GenericCronTrigger")
        .withConfiguration(
            new runtime.Configuration({
                "cronExpression": "0 * * * * ?"
            })).build()
    ]);

runtime.automationManager.addRule(sRule);

If that or something like that won't work, I'm good with the example as it is. Even if that does work, I'm OK with the example as it's currently written so I leave it up to you if you want to change it to be consistent with the other examples (there is some value in that) or keep it different and therefore showing an alternative way to do it.

configuration/jsr223.md Outdated Show resolved Hide resolved
@florian-h05
Copy link
Contributor Author

I've not done file based rules with JS (either version) so I don't know if there is something different. But when I was looking in how to make Nashorn compatible with JS Scripting, the easiest thing was to pull in @runtime and then any call to the OH API classes would go through the runtime variable.

I also thought it would work that way (I was using const { scriptExtension } = require('@runtime')) and it did not.
I will try it again later.

@rkoshak
Copy link
Contributor

rkoshak commented Jul 5, 2022

(I was using const { scriptExtension } = require('@runtime'))

I think that wouldn't be sufficient because I think the presets get imported to the runtime, not the wider script so stuff like SimpleRule and TriggerBuilder wouldn't be available. But that's just an assumption on my part, I've not tried it.

@digitaldan
Copy link
Contributor

Sorry i'm late to the party here, but i wonder if we want to keep this page about "JSR223", or rather just make it "Scripting" and add in the rules DSL to it. As a user i would think its super confusing why its spread around like this across multiple pages, and at the end of the day do they really care about the underlying Java extension , or do they care about the rule languages (Javascript, ruby, DSL, etc...) themselves?

@rkoshak
Copy link
Contributor

rkoshak commented Jul 5, 2022

Sorry i'm late to the party here, but i wonder if we want to keep this page about "JSR223"

Ultimately I think we do need some page to document the actual Java openHAB rules API because there will always be something that a helper library doesn't do or the way it works needs to be worked around for some reason. But it should become an advanced topic and not something that most users ever need to read since they are not using this API directly but instead using the various helper libraries.

However, once we start going down that path we would ultimately end up with a complete reworking of our entire approach for rules documentation in general. I've actually been thinking about this for some weeks now but lack the time to really work on it much.

But were I to start on something like this I'd use the following structure:

  1. Rules Concepts: borrow from the Getting Started tutorial perhaps with more detailed discussion, examples cover most of the supported languages, maybe this belongs in the concepts section
    a. triggers
    b. conditions
    c. actions
    d. comprehensive examples using the respective helper libraries (not the raw API)
    e. rule templates
  2. Actions: These really shouldn't be their own thing, they should be a part of the rules documentation, maybe it should be part of 1. maybe move the persistence actions here too.
  3. UI Rules Reference: Currently not documented anywhere, may be short depending on what's covered in 1. I'd like to see some of the JSON stuff from https://www.openhab.org/docs/developer/module-types/#tie-everything-together-define-rules
  4. Blockly: as currently written
  5. Rules DSL: modified to be more of a reference than explaining rules concepts, goes away when/if this becomes and add-on since it will be part of the add-on's docs
  6. openHAB's raw API: this page with some additions, (e.g. the event Object is missing)
  7. Writing Rule Templates: new content

We need to extract the generic stuff from the current Rules page and move the Rules DSL stuff to its own page. We need to push people to use their respective helper libraries instead of the RAW API. Over all, the Rules docs need to look more like the UI section in the docs.

at the end of the day do they really care about the underlying Java extension , or do they care about the rule languages (Javascript, ruby, DSL, etc...) themselves?

Several of the languages including Groovy and JRules only work through the raw API. Other languages have a third party helper library (i.e. not part of the openHAB project itself) including Jython, Nashorn, and jRuby and users may not want to or be able to install these and instead would rather use the language without the helper library. And not all the helper libraries do as good of a job abstracting the raw API.

Anyway, for this specific PR though, since we are not looking to do a complete reworking of the rules docs and not reworking this specific page, I strongly believe that if we include a JS Scripting example at all, it should use the raw API instead of the helper library. Otherwise it's kind of a pointless example given what this page is about.

When, if the rules docs do get reworked, then there will be lots of other places to include "proper" examples using openhab-js, not to mention the examples in the JS Scripting add-on itself.

@florian-h05
Copy link
Contributor Author

@rkoshak

I think that wouldn't be sufficient because I think the presets get imported to the runtime, not the wider script so stuff like SimpleRule and TriggerBuilder wouldn't be available. But that's just an assumption on my part, I've not tried it.

Object.assign(this, require('@runtime'));
Object.assign(this, require('@runtime/RuleSimple'));
Object.assign(this, require('@runtime/RuleSupport'));

… imports everything needed for the creation of a rule, but I get the following error:

TypeError: invokeMember (create) on org.openhab.core.automation.util.TriggerBuilder failed due to: Unknown identifier: create

So TriggerBuilder is imported, but it somehow cannot be used (according to the JavaDoc there is a create() method.
When I import TriggerBuilder with

const TriggerBuilder = Java.extend(Java.type('org.openhab.core.automation.util.TriggerBuilder'));

everything is working.

FYI: Using scriptExtension.importPreset(…) is not possible as scriptExtension is not available on @runtime, so your example did not work.

I updated the example to use less Java.type.

@florian-h05
Copy link
Contributor Author

@rkoshak
Regarding your approach of how to rework the rules docs: Looks really good to me, maybe you can create an issue with your post above as I would focus on getting this PR merged.

And can you please have a look at my current example? If it‘s good, we are ready to merge.

@rkoshak
Copy link
Contributor

rkoshak commented Jul 6, 2022

Looks good to me. Thanks for doing this. @Confectrician this looks ready to merge and I'm going to open a new issue to talk about reworking the rules documentation.

Issue to rework the rules docs is #1855

Copy link
Contributor

@Confectrician Confectrician left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. 👍🏼

@Confectrician Confectrician merged commit e04c489 into openhab:main Jul 12, 2022
@florian-h05 florian-h05 deleted the jsr223-add-js-note branch July 12, 2022 20:17
@Confectrician Confectrician added this to the 3.4 milestone Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feedback Waiting for feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Difference between JS-Scripting and JS223-Scripting
4 participants