Rules and return codes

[rlkoshak (Rich Koshak)]

@florian-h05

The event object conversion code should be adjustable to work outside of the wrapper.

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.

@florian-h05

I would propose the following, hoping we can agree on this approach:

1. 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.
2. With 3 I see no issue with this either.
3. 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.

@Nadahar

My worry in situations like this is that things are broken “because they don’t matter” right there and then

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.

@Nadahar

I just feel the need to correct claims that “this only applies to Conditions”.

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.

@Nadahar

And, why would you need to enable the wrapper for existing scripts anyway - they already work as they are?

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.

@florian-h05

I actually wasn’t aware that scripted triggers are possible

I can’t imagine how that would work either. Generic Triggers are already pretty comprehensive. But it would be fun to find out.

@Nadahar

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?

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.

@Nadahar

I’m also not sure how widespread scripted Conditions are outside of users that use marketplace rule templates.

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.

@Nadahar

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.

True, but it’s still much better than it was.