-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[jsscripting] Wrapper: Always enable for actions, configurable for conditions #19443
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
Conversation
…e for conditions with default disabled See https://community.openhab.org/t/rules-and-return-codes/166438 for discussion. Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/rules-and-return-codes/166438/60 |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/rules-and-return-codes/166438/65 |
.../main/java/org/openhab/automation/jsscripting/internal/GraalJSScriptEngineConfiguration.java
Show resolved
Hide resolved
...ng/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java
Outdated
Show resolved
Hide resolved
...ng/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java
Outdated
Show resolved
Hide resolved
...ng/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java
Outdated
Show resolved
Hide resolved
...ng/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java
Outdated
Show resolved
Hide resolved
...ng/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java
Show resolved
Hide resolved
...ng/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.../main/java/org/openhab/automation/jsscripting/internal/GraalJSScriptEngineConfiguration.java
Show resolved
Hide resolved
...ng/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Florian Hotze <dev@florianhotze.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates JavaScript scripting behavior: the wrapper is now always intended to be enabled for script actions, while being configurable (and defaulting to disabled) for script conditions via a new directive ("use wrapper") and a renamed configuration parameter. Key changes include replacing the legacy wrapperEnabled config with scriptConditionWrapperEnabled, refining script environment detection, and introducing directive-based wrapper control in scripts.
- Introduces directive-based wrapper selection ("use wrapper[=true|false]") for conditions; actions are meant to always be wrapped.
- Refactors environment detection from "UI-based" to explicit script module/action/condition classification.
- Renames and adjusts configuration and i18n keys to reflect the new behavior.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
File | Description |
---|---|
jsscripting.properties | Renames wrapper configuration keys and updates description text for script condition wrapper behavior. |
config.xml | Replaces wrapperEnabled parameter with scriptConditionWrapperEnabled and changes its default to false. |
OpenhabGraalJSScriptEngine.java | Adds environment classification, wrapper directive parsing, and refactors injection logic. |
GraalJSScriptEngineConfiguration.java | Renames config fields/methods and adds scriptConditionWrapperEnabled handling. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ng/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java
Show resolved
Hide resolved
...ng/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java
Show resolved
Hide resolved
bundles/org.openhab.automation.jsscripting/src/main/resources/OH-INF/config/config.xml
Show resolved
Hide resolved
.../main/java/org/openhab/automation/jsscripting/internal/GraalJSScriptEngineConfiguration.java
Show resolved
Hide resolved
.../main/java/org/openhab/automation/jsscripting/internal/GraalJSScriptEngineConfiguration.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Florian Hotze <dev@florianhotze.com>
5de744b
to
192d550
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
.../main/java/org/openhab/automation/jsscripting/internal/GraalJSScriptEngineConfiguration.java
Show resolved
Hide resolved
.../main/java/org/openhab/automation/jsscripting/internal/GraalJSScriptEngineConfiguration.java
Show resolved
Hide resolved
...ng/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM.
Follow up for #19019.
Discussion in https://community.openhab.org/t/rules-and-return-codes/166438.
let
,const
,class
,function
,return
etc. always for script actions."use wrapper"
"use wrapper"
&"use wrapper=true"
enables the wrapper"use wrapper=false"
disables the wrapper