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

[blockly] Remove Nashorn support & Check for required automation add-on in script edit #2743

Merged
merged 32 commits into from
Sep 16, 2024

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Sep 6, 2024

See https://community.openhab.org/t/blockly-historic-state-average-ending-with-type-error/158191/10?u=mherwege

This PR removes Nashorn support from Blockly and removes all Nashorn specific code from the blocks.

When opening a Blockly script, the UI will tell you if the script needs to be saved again (because the MIME type is for Nashorn or because newly generated code is different from the saved code). It will also warn if the JS Scripting addon is not installed.

The Run Now link from the script editor is grayed out when the required scripting addon is not installed, and will shown a warning when trying to run.

This check is not done when running from the rules page as multiple scripts could be in the rule of which some may be able to run. Having it on the script editor page still makes the issue clearer I believe.

From a support perspective, it now becomes easy to tell people to open the Blockly rule/script in the editor and see if any message pops up.

@mherwege mherwege requested a review from a team as a code owner September 6, 2024 16:09
@mherwege
Copy link
Contributor Author

mherwege commented Sep 6, 2024

Warning when opening the Blockly editor:

image

Copy link

relativeci bot commented Sep 6, 2024

#2261 Bundle Size — 10.82MiB (-0.14%).

fda6c28(current) vs 0546412 main#2260(baseline)

Warning

Bundle contains 2 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes
                 Current
#2261
     Baseline
#2260
No change  Initial JS 1.89MiB 1.89MiB
No change  Initial CSS 576.5KiB 576.5KiB
Change  Cache Invalidation 18.22% 17.96%
No change  Chunks 226 226
No change  Assets 249 249
Change  Modules 2920(+0.03%) 2919
No change  Duplicate Modules 149 149
No change  Duplicate Code 1.8% 1.8%
No change  Packages 96 96
No change  Duplicate Packages 2 2
Bundle size by type  Change 1 change Improvement 1 improvement
                 Current
#2261
     Baseline
#2260
Improvement  JS 9.04MiB (-0.17%) 9.05MiB
No change  CSS 863.35KiB 863.35KiB
No change  Fonts 526.1KiB 526.1KiB
No change  Media 295.6KiB 295.6KiB
No change  IMG 140.74KiB 140.74KiB
No change  HTML 1.36KiB 1.36KiB
No change  Other 871B 871B

Bundle analysis reportBranch mherwege:blockly_nashornProject dashboard


Generated by RelativeCIDocumentationReport issue

@rkoshak
Copy link

rkoshak commented Sep 9, 2024

The Run Now link from the script editor is grayed out when the required scripting addon is not installed, and will shown a warning when trying to run.

This check is not done when running from the rules page as multiple scripts could be in the rule of which some may be able to run. Having it on the script editor page still makes the issue clearer I believe.

Not for this PR obviously, but this makes me wonder if something more generic is worth while. Any rule that depends on an add-on being installed could benefit from such tests and warnings. It would be particularly useful for rule templates. Would something like that be feasible? If so I'll open an issue.

@mherwege
Copy link
Contributor Author

Not for this PR obviously, but this makes me wonder if something more generic is worth while. Any rule that depends on an add-on being installed could benefit from such tests and warnings. It would be particularly useful for rule templates. Would something like that be feasible? If so I'll open an issue.

The graying out of the run link in the script editor would happen for any language, not just for Blockly with javascript. But it is only inside the script editor, not when using the run option from a rule. The challenge there is that multiple actions could be attached to the rule, not all scripts and not all in the same language. It could be grayed out, but should it be grayed out as soon as one of the scripts does not have the language add-on installed? Also, what about rule conditions with scripts?

I think it is a good idea to do a check for installed languages when instantiating a rule from a template. So yes, please, create an enhancement request. I need to investigate further how feasible it is, but it definitely makes sense.

@rkoshak
Copy link

rkoshak commented Sep 11, 2024

It could be grayed out, but should it be grayed out as soon as one of the scripts does not have the language add-on installed?

The rule will error out if run so I would say yes, if the rule is known not to be functional the run options should be grayed out.

Also, what about rule conditions with scripts?

That's more subtle as the conditions do not get run when you hit the play button. So from that perspetive the rule would still be functional so I wouldn't gray the button out in that case.

But maybe this is all simpler than that. If you have an inline script (action or condition) with an unknown MIME type (presumably becuase the add-on isn't installed) the status of the rule becomes "UNINITIALIZED" with a Handler Initializing Error.

image

And there is already some checking in place becuse if you click the play button in those cases it throws up a toast saying it can't run the rule.

However the error message presented isn't particularly informative. Also, the error checking isn't complete because if you load the rule into the developer sidebar it will act like it ran the rule even though it didn't and there's no error.

I think this last part with the developer sidebar is a bug. Improving the error message and/or graying out the play button would be an enhancement. I'll split it into two issues accordingly.

@florian-h05
Copy link
Contributor

@rkoshak Thanks for creating the two issues, I have already resolved one 🚀
@mherwege Can you please rebase? Your block improvement PR caused some conflicts here.

@florian-h05 florian-h05 added enhancement New feature or request main ui Main UI labels Sep 13, 2024
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

@florian-h05 rebase done

@florian-h05 florian-h05 removed the request for review from a team September 16, 2024 13:47
Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks for the huge clean-up and the added stuff!
I have changed a few minor things, only fixes and code style.

As the changes to the code generation are only the removal of the Nashorn code generation; I don't think additional testing of the code generation is needed and I will therefore merge this PR now.

In case there are issues, let's fix them in a follow-up - this PR already has enough changed lines.

@@ -371,6 +384,13 @@ export default {
isMimeTypeAvailable (mimeType) {
return this.languages.map(l => l.contentType).includes(mimeType)
},
mimeTypeDescription (mode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather have these in a asset file, where we can also link doc links etc ... so I have changed that.

@florian-h05 florian-h05 added this to the 4.3 milestone Sep 16, 2024
@florian-h05 florian-h05 changed the title [Blockly] Remove Nashorn support [blockly] Remove Nashorn support & Check for required automation add-on in script edit Sep 16, 2024
@florian-h05 florian-h05 merged commit 6678728 into openhab:main Sep 16, 2024
8 checks passed
@mherwege mherwege deleted the blockly_nashorn branch September 16, 2024 15:45
@stefan-hoehn
Copy link
Contributor

stefan-hoehn commented Dec 15, 2024

@mherwege @florian-h05 This is a major change. I only noticed this now due to Florian asking me to write something about all Blockly PRs. No offense but I would have appreciated to be mentioned at least. 😢🙏🏻
Changes to Blockly in particular require doc updates and I wonder if this has been mentioned and updated in the docs?

@mherwege
Copy link
Contributor Author

@stefan-hoehn My apologies. It was never my intention to bypass you. I wrongly assumed when I named the PR with [blockly] ... you were monitoring it.
I indeed should have a look at the documentation again to remove references to Nashorn at this time and clearly state GraalJS needs to be installed. I don't think we necessarily need to explain the check it does and the message it returns when the check fails. That should be self explanatory. If it isn't, it should be adapted.

@stefan-hoehn
Copy link
Contributor

 "...never my intention to bypass you. "

I know 👍🙏

florian-h05 added a commit that referenced this pull request Dec 17, 2024
florian-h05 added a commit that referenced this pull request Dec 17, 2024
florian-h05 added a commit that referenced this pull request Dec 17, 2024
florian-h05 added a commit that referenced this pull request Dec 17, 2024
#2928)

Regression from #2743.

Signed-off-by: Florian Hotze <[email protected]>
(cherry picked from commit 511b949)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants