To be sure, it is not something many users will have a feel for initially and there will be dozens of “why doesn’t my rule work?” questions on the forum, but the solution will be an easy answer and set of steps to guide them.
This is exactly what my “directive” suggestion does. I really don’t understand why there’s so much opposition to this. There will still be a default, that can change whenever you see fit - but the directive would allow the user to override the default. Nobody will ever have to use the directive, but it’s a way to make sure things keep working if that’s your desire.
I really think people should think more carefully about all the problem you make for people when breaking things constantly. I get that it’s frustrating to have to wait (for the developer), but understand that there are a lot of people that don’t follow what happens internally, and all they see is that things break, which they then find out is deliberately, not a bug, and won’t be “fixed”.
Making the wrapper available as a (per script) opt-in now is fine, but I’d say it’s unwise to force it on people until a major release. Even with a major release, I’d personally like if such breaking things weren’t changed unless absolutely necessary.
I’ve been trying to tell you that this isn’t really true. Actions (and Triggers AFAICR) can also return values. It’s just that this isn’t really used much, and most people don’t seem to know how to use it, but it’s there, and it will break this just as much as with Conditions. The difference is just that Conditions don’t work at all without a return value, unlike Triggers and Actions, so the problem is immediately apparent.
Again, it applies to all scripts, and I really don’t get how you think such an “algorithm” should be anywhere close to “smart enough” to do a reliable job.
That is only true as long as you all keep dismissing the “directive”. It would enable transitioning them one by one.
It’s this actually two different things? When looking at the code, it seemed to me like what type of events to send and the wrapper are independent. But, the relationship might be that without the wrapper, the script can’t “understand” the JS event object?
I don’t understand all the creative suggestions for creating different ways to configure this instead of just putting the “configuration” to enable it in the script itself, aka use wrapper=[on|off].
With a directive, the answer will be easy: "Add "use wrapper=false" to the top of your script. Without it, a whole lot of explaining about return statements, how they used to be automatically evaluated and how it works when return is explicit is needed. I’m not so sure that everybody will catch this right away, or appreciate that they have to sit down and go through all their rules at that point or be forced to downgrade (if they even have a backup from before the upgrade).
I don’t know the underlying java at all, so, as long as @florian-h05 continues to remain skeptical about the use of a directive for reasons I am not qualified to evaluate, I’m happy to continue to brainstorm.
Any of the rule-by-rule options will get misused by users. Without an explanation of “why” you have to use a directive or a configuration or a whatever, there will always be “my rule didn’t work, so I found a post that says [do X] and now I get a different error”.
I did say
but the difference is not whether the user has to understand, merely how many clicks it takes for them to fix their problem and if they even know where to look.
I don’t think his reservation is technical - I can whip up a PR that does this in no time. In fact, the only “challenge” is to come up with a regex that is reasonably robust, I’d think that the “directive” should only apply if it’s at or close to the top of the script, not inside a function etc. The regex would have to take care of that. The rest is extremely easy to do.
I can’t know, but I suspect that he might be reluctant because he wants it to go live “now”, and don’t like the suggestion of waiting for 6.0 to enable it by default. But that isn’t really tied to the “directive” idea, I’d say that the “directive” would be useful regardless of when the default changes.
You could even keep the global wrapper on/off configuration that is implemented now, and the “directive” would still be helpful. Because, those scripts that include the directive are independent of the global setting, the rest complies with the global setting. So, you can “transition in your own pace”, or you can, for example, make templates that are compatible across versions with different defaults.
Any option at all can be “misused” if people insist on remaining ignorant. My idea isn’t that the return statement change shouldn’t be documented/explained, but that if you can add a simple directive to retain previous behavior, users can have the time to read the explanation and adapt, without everything stopping to work until they do.
All I’m saying is that is a feature of ANY option that allows individual rule configuration, not just your directive proposal.
I’m not saying that a directive isn’t a good option. But that it is not unique for most of the characteristics that I’m able to evaluate and so not inherently better than some of the other options from my perspective.
1 Like
Because it’s not actually used with Script Actions today and it does nothing, no one is using it. I’m not even certain we could use it if we wanted to from JS. So no existing rule that has a JS Action that works today will break when the wrapper is enabled because of a lack of a return statement in the Script Action.
When and if there is a use for return values from JS Script Actions than yes, we will have to address that problem at that time. But this is not a problem that will impact any JS Scripting users today. It is not a hindrance to adopting the use of the wrapper.
As long as @florian-h05 says he’s not going to do it and is against the idea, it’s not going to be done. I’m neither arguing for or against it. Just recognizing that even the best idea in the world does no one any good if it isn’t feasible.
But, I kind of don’t like the need to add a directive to every one of my scripts in order to use the wrapper, which should eventually become the default anyway.
[rant]One of the frustrating things about changes like these is that so few of the people who have strong opinions about it actually use managed JS Scripting rules day-to-day. I think @JustinG is the only other person on this thread right now who does. It’s very hard to convey sometimes how some simple change like “just add a directive to all your scripts” becomes a burden on managed rules users.
It seems OK to start, but I can’t just put this directive at the top of one .js file with several rules and be done. No, I have to add that line to the top of each and every individual script. I might have two or more scripts in a single rule. It’s the same line over and over and over and over again.
The whole reason we have automatic injection of the openhab-js library was because I was able to show that a significant percentage (something like 15-25% IIRC) of all the code a managed rules user would end up writing would have been just importing the library. A directive is a step back in that direction.
One of the main reasons I use JS Scripting today instead of jRuby was at the time the jRuby developers didn’t care about managed rules but the JS Scripting developers heard me out (this was before you’re efforts @jimtng). I don’t want to see us backslide into requiring a bunch of proforma on each and every managed script to use a major feature which eventually needs to be the default setting.
It was always a travesty that managed JS script users had a different environment from file .js users. I’m not keen on preserving that situation forever.
[end rant]
You must have the wrapper enabled to get the enhanced JS event. The wrapper is what makes that possible in the first place.
I currently have 88 JS scripting scripts in my relatively modest set of rules. Many users will have hundreds. This is just to present the scale of the issue at hand.
-
Users must have a way to choose when to adopt the wrapper. Whether that’s globally or on a script-by-script basis is up for debate.
-
Having a way to adopt it on a script-by-script basis helps the users transition to using the wrapper (it’s not all or nothing). However, all the proposals mentioned thus far also impose a large burden on those users.
-
Having a way to disable the wrapper globally at least lets users continue on as they always have, but that makes it harder for the wrapper to become the default/only approach later on at some point.
-
Not making the wrapper the default at some point comes with a large maintenance burden. We get no savings from the docs because we need to maintain the “wrapper off” docs forever. We get no savings in the code because we need to maintain the “wrapper off” branches and conditions and such forever. We only add to the maintenance burden.
-
Forcing the users to make changes all at once is also a large burden. It’s especially large if we don’t give them any tools to help automatically identify what changes need to be made (e.g. errors in the logs with explanations of which rule and what to change, pop-up in MainUI listing all the problems like is done with orphaned links and semantic model problems, upgradeTool finding and listing all the managed rules with conditions, etc.).
Our challenge is to find least burdensome path for everyone to get us to the point where the wrapper being enabled is the default for most if not all users. I don’t know that we’ve found that yet. There may not be one at all.
IMO for actions there is no practical concern.
There is no documentation about how to access the return value of an action, and without the ability to use the return keyword to explicitely return something its even more unclear what is returned from an action.
I think no one really uses this for JS Scripting.
I think that idea that the wrapper is required comes from the fact that I originally thought that, and that the current code is designed to work only that way. The event object conversion code should be adjustable to work outside of the wrapper.
A directive could be implemented relatively simple, just check if the script includes or starts with the directive string.
My concern is not the implementation, but instead:
I am primarily trying to find a solution that brings the most benefit for the average user. IMO for actions that would be to always enable the wrapper to allow people to early exit from execution, and the other stuff mentioned above. And as said above, I don‘t think anyone has really returned values from JS actions, what‘s not documented is likely to be not widely used.
At the moment yes, but I think the conversion can also work without the wrapper if I adjust the code.
I would propose the following, hoping we can agree on this approach:
- Always enable the wrapper for script ACTIONS. As stated multiple times above by Rich and me, the change in return value from actions is neglectable, since it is highly unlikely anyone used it all.
- Do not enable the wrapper for CONDITIONS by default (yet). This avoids breaking things.
- Get the event object convrsion working with and without the wrapper.
How we to the „migration“ path of conditions to use the wrapper as well is a different thing, that we can decide on in a next step.
2 Likes
My worry in situations like this is that things are broken “because they don’t matter” right there and then, and over time, it gets harder and harder to actually make it work again. If somebody comes up with some bright idea that relies on this mechanism, the fact that something “major” like jsscripting has left it broken a long time ago and then built upon that, might pose such an obstacle that actually using what’s supposed to have been there the whole time to solve a problem, becomes “effectively impossible”. It often costs so little extra to take things into account initially, but as things evolve on top of previous decisions, it becomes “cemented” and the cost suddently becomes huge. But, this is more of a philisophical issue, so I’ll leave it at that. I agree that this is probably not a problem at the moment, I just feel the need to correct claims that “this only applies to Conditions”.
That was never my suggestion either. The directive was only meant as a way to override whatever is the default behavior where it’s needed. So, for all the scripts that will work either way, there’s no need to do anything. Whether you need to use the directive to enable or disable the wrapper depends on what the default or global setting is.
What the directive brings to the table is the ability to control, on an individual script basis, exactly how that one will work. It means that you would be able to make rule templates where you turn the wrapper off explicitly, to make sure that the same rule template will work on previous and current versions of OH. Since the directive is only a string that doesn’t “do” anything in the script, it should be “harmless” to previous versions that don’t know the directive.
You’re losing me here, I don’t understand why this would be needed. I never meant that the directive should be the only “source of control” of the wrapper, I meant that it would be a valuable tool to make sure that things could keep working despite changing defaults or global settings. And, why would you need to enable the wrapper for existing scripts anyway - they already work as they are? Isn’t the benefit that it should be easier to handle events and that you can use “more modern” syntax? This would only be a benefit when you write something new, as far as I can understand.
This again assumes that the directive would have to be used on a large scale, I’m not sure that I can see why it would.
To repeat: My idea with the directive is that:
- It’s a way to override the “rule” that otherwise apply on an individual script basis. The reason for doing that can be cross version compatibility, lack of time to digest how to adapt to the wrapper, or yet unforeseen situations where the wrapper turns out to cause trouble.
- The directive was never meant to be the only way to control the wrapper. I initially suggested that it could replace the global setting, but that’s not necessary. The global setting can change its default whenever you deem appropriate, @rlkoshak said earlier that the default probably shouldn’t change until 6.0, which is why I went with this as an example, but that decision is completely unrelated to the use of the directive.
- A global setting or a default that the users don’t control has one big drawback, it applies “across the board” with no way to allow for exceptions. That might force some to turn the wrapper off globally just because of a few scripts where there are some challenges. With the directive, you would have the freedom to set the global setting to whatever was the most convenient for the majority of the cases, and still have a different behavior for the few where that doesn’t fit.
- As far as I can tell, the directive is the only way you can use the wrapper in a rule template if the global option exists, without going down some hugely complicated route of trying to detect the wrapper. If you want to make a rule template that will only work >= 5.1, use the directive to enable the wrapper, and then use it, knowing that it will work regardless of what the global setting is.
I believe that a global setting + the option to override individual scrips is the most flexible and less burdensome way there is.
Does this imply that you want to get rid of the global setting, and instead “force” the wrapper on for Actions and Triggers, and off for Conditions? (Before the protest: I know that the current practice isn’t to use scripted Triggers, but from what I can tell, the system supports it, so it should be included for completeness).
You need to add a return something to the action and you have your return value. The change caused by the wrapper is not that the ability to return something is gone, the change is you need to explicitly return something using the return statement. There is no implicit „the last statement is considered as return“ anymore.
So it’s not broken, it’s IMO even more understandable. The word break more refers to we don’t break the existing scripts since there is likely no one using it.
I actually wasn’t aware that scripted triggers are possible, but wrapper on for them. It’s all new code so we can enforce the new environment there.
WRT to the conditions: No, I don’t want to remove the setting. I like your suggestion of having a global default and the directive, this would allow to step by step migrate each condition to using the wrapper, you can turn off the default and use the directive.
Especially thinking of rule template authors, they could enable the wrapper as they like.
There might get additional directives added anyway, there are feature requests for debugger support and then need to enable it per script engine and assign it its own port.
1 Like
I haven’t actually tested it, but it seems to me that all the necessary parts are there for Triggers as well. My reasoning has been that it probably have never come up, because to trigger, you need something to trigger on, and unless that is the clock (which you could always do), you’d probably need to subscribe to OH events or something else so that you’d have any input to work with. And, since there seem to be Triggers that cover most of the standard stuff that can happen i OH anyway, what would be the point of doing it with a script? I guess the only real use case for this is if you have some input that you want to react to that is available to the scripting language but not to OH itself. I don’t know if such a situation exists.
When it comes to making the global setting apply only to Conditions: I assume that the rationale is that since Conditions are the only place we’ve seen “problems” this far, the setting could default to “off” for now and the wrappers could be enabled elsewhere with little consequence.
To that I’d like to add that I would at least still let the directive apply to Actions and Triggers - if it turns out that there is some unforeseen problem that pops up in the future, there’s an easy way to deal with it.
I’m also not sure how widespread scripted Conditions are outside of users that use marketplace rule templates. My impression is that Conditions is used very little in general. Those users that have reported problems, are they all users of rule templates, or have they actually written scripted Conditions themselves? I’m just wondering how much problems making the global setting enabled by default would really have.
Sadly, since I’ve never managed to finish the versioning of the marketplace, users would still face potential problems even if the marketplace rule templates were edited to include the directive, they would still have to uninstall and reinstall the rule template, and then regenerate the rules, for the changes to apply.
1 Like
I was under the impression the wrapper was required.
So we could get the enahanced JS event Object independent from the wrapper? That might be a middle path. Then users can adopt the new event Object which just complains in the logs with the old code, without using the wrapper which could break their code.
From a template author perspective I still need a way to detect both though. The event I can detect by looking for event.raw I think, but I don’t know how I’ll be able to detect if the wrapper is present.
- I see no issue with that. There should be no errors. User will just suddenly have the option to return early and use const and let. It shouldn’t even be noticable.
- With 3 I see no issue with this either.
- I think this opens up opportunities to make the transition better. We could even just make the wrapped event the default. That’s not a breaking change. It still works, just with lots of deprecation warnings.
But the changes proposed doesn’t break Script Actions from returning a value. In fact, the wrapper provides the only way that a JS Scripting user could ever use it in the first place. The wrapper makes it easier for JS rules to use this feature in the future if not providing the only way it could ever be used at all.
And because a JS user cannot use it now, imposing the wrapper on script actions won’t break anything.
The part of the changes that break things only applies to Conditions. I don’t think anyone is arguing that Script Actions are wholly irrelevant. But it is the fact that the wrapper won’t impact any JS Script Action that exists today so that’s not a problem that will hit end users.
To simplify and/or improve the long term maintainability of a rule. Being able to return early (fail fast) could be a huge improvement to some rules. Because I want to.
But it sounded like to not adopt the wrapper I would need to add a directive to the script.
As long as we don’t have to use it everywhere I’m good.
I can’t imagine how that would work either. Generic Triggers are already pretty comprehensive. But it would be fun to find out. 
There is also GenericTrigger which is a way to kind of invent your own trigger with a filter on all the events.
For example:
triggers:
- id: "3"
label: A Thing Changes Status
description: Triggers when any Thing changes status
configuration:
topic: openhab/things/**
types: ThingStatusInfoChangedEvent
source: ""
payload: ""
type: core.GenericEventTrigger
That triggers on all ThingStatusInfoChangedEvents for all Things.
I would expect a script trigger to get something like what’s in the payload (see below) and then have some code to filter down to just the ones it cares about. But that’s a lot of heavy processing.
As an aside, something might be broken with GenericEventTriggers right now as the event.payload doesn’t seem to be there in the wrapped event. Event event.raw.payload doesn’t seem to work either (fyi @florian-h05, there might be a new issue in the future). I haven’t had a chance to look into it more closely to see if it still works with the wrapper/JS event turned off nor look at the changes to the code. But access to the payload is the only way to get at stuff that isn’t in the JS event, such as in this case the previous thing status description and the new thing status description.
Enough people use them to warrant our concern. It’s not just rule template users. I’ve seen numerous examples of users who have script conditions. It’s really the only way to have anything even remotely complex like “this Item is ON and that item is OFF”. Basic conditions only support OR, not AND.
True, but it’s still much better than it was.
1 Like
If the directive is implemented, you won’t have to. Can can dictate it: If you target versions prior to 5.1, turn the wrapper off with the directive, and you know that it’s off. If you only target >= 5.1, turn the wrapper on with the directive, and you know that it’s on.
Completely off-topic, but I’d just like to mention it since I’m not sure if @rlkoshak ever became aware: Even though the “new YAML format” rules and rule templates PR is stuck, I did manage to get parsing of rule templates in the “old YAML format” (aka. the one in use on the marketplace) and in JSON format merged earlier. So, you can test rule templates locally today, by putting *.json or *.yaml files in conf/automation/templates. I just am not sure if you ever became aware of this. I haven’t documented it, since it’s all set to change anyway, but it’s there and it works. It’s using the exact same code for the parsing as the marketplace does, so you should be able to trust that what works there will work on the marketplace.
I wasn’t aware of this. Thanks for letting me know. It will help me a lot test these darn things before publishing them. I’m gonna have to make some minor edits at least to several of my templates and I’ve only done two so far.
I also think local rule templates are a feature that users might want to use too. I’m sure there are plenty of people who would rather write a simple rule once then duplicate it for different sets of Items than copy/paste/edit and/or publishing a rule template to the marketplace.
I’ll have to experiment with that tomorrow.
Exactly. Wrapper only turned off for conditions though, just to note it.
event.raw? This also includes what is received, i.e. the plain Java event.
I will then prepare the required PRs in the next days:
- Inject the module type into the context (core): [automation] AbstractScriptModuleHandler: Inject module type ID into context by florian-h05 · Pull Request #5054 · openhab/openhab-core · GitHub
- Make event object conversion work independent from wrapper (add-on): [jsscripting] Make event object conversion work independent of wrapper by florian-h05 · Pull Request #19429 · openhab/openhab-addons · GitHub
- Always enable wrapper for actions & Make it only configurable for conditions, default to off there (add-on PR)
- Implement the directive. We can easily make it work for both actions and conditions to have a way of disabling the wrapper if required, though this is not expected. I am against documenting this directive for actions though, will only cause confusion. (add-on PR)
The add-on PRs might be combined into a bigger one.
2 Likes