Move (most) javascript out of jelly files #76

rahulsom · 2023-06-23T16:25:58Z

We're limited in what we can do in jelly files.
There are tests that use HtmlUnit and it does not support enough modern JS features.

This change will allow us to use modern JS to write code, and transpile it to something that all the older browsers support and HtmlUnit does not error out.

This is built on top of #75

Testing done

This is a pure refactor. No new functionality is added. All existing tests pass.
The code that was moved from jelly to js has a lot of dependencies.
We'll need more refactoring before we can test it independently.

Submitter checklist

Give feedback

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue
Options

basil · 2023-06-23T20:00:33Z

pom.xml

+                <groupId>com.github.eirslett</groupId>
+                <artifactId>frontend-maven-plugin</artifactId>
+                <version>1.12.1</version>


This configuration is already provided in the parent POM if you check in a blank .mvn_exec_node file:

https://github.com/jenkinsci/plugin-pom/blob/a60e3494e8447aefed562deace5d222e4777413a/pom.xml#L1128-L1198

basil · 2023-06-23T21:03:13Z

You don't have to switch to NPM, either: the plugin parent POM has a corresponding .mvn_exec_yarn feature.

rahulsom · 2023-06-23T21:12:17Z

You don't have to switch to NPM, either: the plugin parent POM has a corresponding .mvn_exec_yarn feature.

I don't have strong feelings one way or the other. I'll leave it at NPM for now unless @basil or @kinow feel strongly about either tool.

basil · 2023-06-23T21:14:26Z

Core uses Yarn, but for individual plugins it is up to the maintainer.

kinow · 2023-06-25T08:36:47Z

(sorry, other merges caused some conflicts here)

rahulsom · 2023-06-25T17:10:54Z

NP. I'll clean this up after #75 is merged.

We're limited in what we can do in jelly files. There are tests that use HtmlUnit and it does not support enough modern JS features. This change will allow us to use modern JS to write code, and transpile it to something that all the older browsers support and HtmlUnit does not error out.

rahulsom · 2023-07-04T16:59:55Z

@kinow This is now ready for review. Thanks!

kinow · 2023-07-04T17:46:26Z

src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6

@@ -363,7 +363,7 @@ var UnoChoice = UnoChoice || ($ => {
                $(".behavior-loading").show();
                // start updating in separate async function so browser will be able to repaint and show 'loading' animation , see JENKINS-34487
                setTimeout(() => {
-                   _self.cascadeParameter.update();
+                   _self.cascadeParameter.update(false);


The other changes were easier to review. This one is the only one I didn't understand straight away. Could you explain why the false added here?

Sorry about that. The default value was undefined which was interpreted as false. This makes it explicit.

No need to apologize! I suspected you would have a reason for doing that. Thanks for the explanation. LGTM! 💥

kinow · 2023-07-04T19:17:59Z

src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6

@@ -363,7 +363,7 @@ var UnoChoice = UnoChoice || ($ => {
                $(".behavior-loading").show();
                // start updating in separate async function so browser will be able to repaint and show 'loading' animation , see JENKINS-34487
                setTimeout(() => {
-                   _self.cascadeParameter.update();
+                   _self.cascadeParameter.update(false);


No need to apologize! I suspected you would have a reason for doing that. Thanks for the explanation. LGTM! 💥

Changes for #76

kinow · 2023-07-04T19:21:13Z

@rahulsom thank you for your recent contributions. I think we got all your pull requests reviewed and merged, right? Shall I cut a new release with all the current changes in master? Or do you have anything you'd like to include?

rahulsom · 2023-07-04T19:23:35Z

Thanks @kinow ! I'm working on one last PR to remove the dependency on prototype. That should get this plugin ready for the next LTS.

kinow · 2023-07-04T19:37:01Z

Brilliant! Thanks heaps, I have a meeting with @imoutsatsos tomorrow to chat about Jenkins and about this plug-in -- he's the person who had the idea and initial design of how this plug-in should work, as well as designer of most of the functionality that he uses at his $job. Great timing for a new release soon before summer starts over here. The next PR's are a lot easier to review now that we have a lot more (and better) tests. Bruno.

eplodn · 2023-07-31T11:47:56Z

@rahulsom @kinow Could it be that this change has introduced JENKINS-71706 and JENKINS-71724?

Experiencing

[Log] Values retrieved from Referenced Parameters:  (UnoChoice.js, line 1)
[Error] SyntaxError: Unexpected token ','
	(anonymous function) (build:1067)
[Error] Failed to load resource: the server responded with a status of 403 (Forbidden) (UnoChoice.js.map, line 0)

Function:

                        <script type="text/javascript">
                        // source, references table
                        var referencedParameters = Array();


>>>                        UnoChoice.renderDynamicRenderParameter('#choice-parameter-12948928667198118', 'HEADING_FRONTEND_HIDDEN', 'choice-parameter-12948928667198118', referencedParameters, dynamicReferenceParameter);
                        </script>

kinow · 2023-07-31T13:10:33Z

@eplodn possibly, this SyntaxError could be because of this one, I think it was the only major change in JS, and the #79 , but that one wouldn't result in a SyntaxError, I think.

rahulsom · 2023-07-31T14:56:21Z

JENKINS-71706 claims to be on 2.6.5. This change made it into 2.7.

eplodn · 2023-07-31T15:05:44Z

I have attached a minimal pipeline over at JENKINS-71724 that constantly works on 2.6.5 and mostly fails on 2.7.

Something about the parameters is strange, sometimes i see

                            UnoChoice.renderCascadeChoiceParameter('#choice-parameter-14260437059427953', , 'LEASE_TIME', 'choice-parameter-14260437059427953', 1, 'choice-parameter-14260437059427953', referencedParameters, cascadeChoiceParameter);

(note two , in a row.)

Sometimes I see


                        UnoChoice.renderCascadeChoiceParameter('#choice-parameter-14261231071587458', false, 'LEASE_TIME', 'choice-parameter-14261231071587458', 1, 'choice-parameter-14261231071587458', referencedParameters, cascadeChoiceParameter);

(note false at the previously empty place.)

Going into Configure - Save toggles it between a working and non-working state on 2.7.

basil reviewed Jun 23, 2023

View reviewed changes

rahulsom force-pushed the move-js-out-of-jelly branch from 66862ae to fe12cfd Compare July 3, 2023 23:07

kinow reviewed Jul 4, 2023

View reviewed changes

kinow approved these changes Jul 4, 2023

View reviewed changes

kinow merged commit 85324da into jenkinsci:master Jul 4, 2023

kinow added a commit that referenced this pull request Jul 4, 2023

Update CHANGES.md

5f9de06

Changes for #76

rahulsom deleted the move-js-out-of-jelly branch July 4, 2023 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move (most) javascript out of jelly files #76

Move (most) javascript out of jelly files #76

rahulsom commented Jun 23, 2023

Submitter checklist

basil Jun 23, 2023

basil commented Jun 23, 2023 •

edited

Loading

rahulsom commented Jun 23, 2023

basil commented Jun 23, 2023

kinow commented Jun 25, 2023

rahulsom commented Jun 25, 2023

rahulsom commented Jul 4, 2023

kinow Jul 4, 2023

rahulsom Jul 4, 2023

kinow Jul 4, 2023

kinow Jul 4, 2023

kinow commented Jul 4, 2023

rahulsom commented Jul 4, 2023

kinow commented Jul 4, 2023

eplodn commented Jul 31, 2023

kinow commented Jul 31, 2023

rahulsom commented Jul 31, 2023

eplodn commented Jul 31, 2023 •

edited

Loading

Move (most) javascript out of jelly files #76

Move (most) javascript out of jelly files #76

Conversation

rahulsom commented Jun 23, 2023

Testing done

Submitter checklist

basil Jun 23, 2023

Choose a reason for hiding this comment

basil commented Jun 23, 2023 • edited Loading

rahulsom commented Jun 23, 2023

basil commented Jun 23, 2023

kinow commented Jun 25, 2023

rahulsom commented Jun 25, 2023

rahulsom commented Jul 4, 2023

kinow Jul 4, 2023

Choose a reason for hiding this comment

rahulsom Jul 4, 2023

Choose a reason for hiding this comment

kinow Jul 4, 2023

Choose a reason for hiding this comment

kinow Jul 4, 2023

Choose a reason for hiding this comment

kinow commented Jul 4, 2023

rahulsom commented Jul 4, 2023

kinow commented Jul 4, 2023

eplodn commented Jul 31, 2023

kinow commented Jul 31, 2023

rahulsom commented Jul 31, 2023

eplodn commented Jul 31, 2023 • edited Loading

basil commented Jun 23, 2023 •

edited

Loading

eplodn commented Jul 31, 2023 •

edited

Loading