-
Notifications
You must be signed in to change notification settings - Fork 102
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
refactor: Replace synchronous proxy with standard proxy #79
Conversation
f55a202
to
738a194
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.
Very nice! Highly appreciate this contribution.
The test fails on 2.413 when run with mvn clean verify -Dtest=org.biouno.unochoice.UiAcceptanceTest -Djenkins.version=2.413
. The same test passes on 2.413 on the main branch. Console logs:
UnoChoice.js:1 Values returned from server: undefined
build:1 Uncaught (in promise) SyntaxError: "undefined" is not valid JSON
at JSON.parse (<anonymous>)
at UnoChoice.js:1:8886
at bind.js:56:33
If the server is returning JSON (which was the original use case supported by Stapler), then the client is expected to call |
738a194
to
456a844
Compare
Thanks! That is cleaner. |
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.
Looks good! I tested the result both with and without Prototype and it seems to be working without Prototype. Just two minor comments before approving this.
src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6
Outdated
Show resolved
Hide resolved
604ebc4
to
1e234e0
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.
Very nice!
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.
The plugin is still using inline javascript. That should loading be changed to conform to CSP.
But this is probably something for a separate PR.
src/main/resources/org/biouno/unochoice/CascadeChoiceParameter/index.jelly
Outdated
Show resolved
Hide resolved
src/main/resources/org/biouno/unochoice/DynamicReferenceParameter/index.jelly
Outdated
Show resolved
Hide resolved
1e234e0
to
ed3398b
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.
Didn't have time to check out the code and play with it tonight, but it's looking good! Left a few questions, mostly me learning what's new in Jenkins API & dev :) Thanks!
src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6
Outdated
Show resolved
Hide resolved
} | ||
function decrementLoadingCounter() { | ||
loadingCounter--; | ||
} |
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.
I thought it was going to be used to check for consistency, or maybe for the tests, but I couldn't locate where the loadingCounter
value is being read. Is that somewhere in this file? Maybe GitHub UI is not showing it in the diff (didn't have time to check it out tonight, sorry).
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.
I did try it. I got rid of it. I'll remove this code. Thanks for the extra pair of eyes on this.
@@ -2,6 +2,7 @@ | |||
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler"> | |||
${it.parameters.clear()} | |||
<st:include page="/org/biouno/unochoice/common/choiceParameterCommon.jelly"/> | |||
<st:bind value="${it}" var="cascadeChoiceParameter"/> |
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.
👍 I think I used the other ugly syntax to fix warnings on my IDE (Eclipse + WebDev at that time, I think?). Do VSCode/WebStorm understand this? Maybe there's a stapler plug-in that I can install?
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.
I'm on IntelliJ and it seems to understand it.
Also, now most of the JS is out of this file, so there isn't much context left.
I do have this plugin - https://github.com/jenkinsci/idea-stapler-plugin
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.
That's the reason I changed this.
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.
Right, inline syntax is not going to work with Content-Security-Policy prohibiting inline scripts. See JENKINS-60865 for context.
964274e
to
44d8b2e
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.
These changes look all good to me, @rahulsom !
I don't have an installation with jobs to test it, but I know @imoutsatsos maintains probably the largest and most complex series of jobs with active choices parameters 🙂
@imoutsatsos would you be able to test this change? If everything works for you we can merge it. No need to look at the code, just try the existing jobs you have, checking if you get any JavaScript errors, or parameters that were supposed to be rendered correctly not being displayed, displaying wrong values, etc.
Thanks!
I will do my best to test the updated AC plugin @bruno P. ***@***.***> but it could take me several days given that I will have to set a test harness separate from my current environments. Is there a minimum Jenkins LTS version required?
Sent from my T-Mobile 5G Device
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: Bruno P. Kinoshita ***@***.***>
Sent: Monday, July 10, 2023 1:12:55 PM
To: jenkinsci/active-choices-plugin ***@***.***>
Cc: Ioannis K. Moutsatsos ***@***.***>; Mention ***@***.***>
Subject: Re: [jenkinsci/active-choices-plugin] refactor: Replace synchronous proxy with standard proxy (PR #79)
@kinow commented on this pull request.
These changes look all good to me, @rahulsom<https://github.com/rahulsom> !
I don't have an installation with jobs to test it, but I know @imoutsatsos<https://github.com/imoutsatsos> maintains probably the largest and most complex series of jobs with active choices parameters 🙂
@imoutsatsos<https://github.com/imoutsatsos> would you be able to test this change? If everything works for you we can merge it. No need to look at the code, just try the existing jobs you have, checking if you get any JavaScript errors, or parameters that were supposed to be rendered correctly not being displayed, displaying wrong values, etc.
Thanks!
—
Reply to this email directly, view it on GitHub<#79 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AA2ACYWAN4IRKLVYUQRQ5ALXPQZZPANCNFSM6AAAAAAZ6DEUEQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
For this version of the plugin to run, you need There isn't an LTS with the Prototype removal changes yet. You can use |
Thank you @imoutsatsos ! Just to confirm, the snapshot version you mentioned was built from this branch/pull request? If so we might be ready for a release! Thanks! |
@rahulsom could you base, please? Thanks 🙇 |
44d8b2e
to
5815d29
Compare
@kinow Rebased. We're ready for merging. Thanks! |
Thanks @rahulsom . Just waiting for Ioannis confirmation that the tests used this branch and then we are ready to merge. |
The code in `makeStaplerProxy2` used `prototype.js` to create a synchronous version of the standard proxy that comes with Jenkins Core. This code switches back to the standard proxy. That requires making the code execute in the correct order when there were dependencies. That is achieved by using `async`/`await` in the `.es6` code and transpiling it down to use Promises so HtmlUnit tests continue to work. Refs: JENKINS-71365 Co-authored-by: Basil Crow <[email protected]>
5815d29
to
8fc61b5
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.
Thank you for testing it, @imoutsatsos ! And thanks for the great work (again!) @rahulsom ! Merging.
@rahulsom , I think now we have everything merged, so I am planning to cut a release tomorrow with everything on |
This is being reverted due to issues caused when parameters are handled out of order. The user scripts may use document.getElementById, for instance, to access the elements of other parameters, which fails with this change as parameters may be rendered in any order with a proxy. In order for this to work, there would have to be a JS layer option for users to attach their callbacks/code executed once all parameters have been loaded, or after a certain parameter has been loaded (which is possible in Vue, React, Svelte, etc., but not in pure Jenkins UI + JS). |
The merge request mentions JENKINS-71365 (prototype.JS is removed from Jenkins core) but reverting this should not impact that. What we are doing is removing the async calls and promises, and instead using the synchronous code again. This is so that Active Choices parameters are rendering in order as they were before. Whilst rendering them asynchronously seemed like a good idea to provide a more responsive UI, in reality we oversaw an important aspect: users write their scripts relying on the order parameters are loaded. Users wrote their code using document.getElementId, for instance, which worked previously as users would put the parameter doing that after the parameter from which the ID was being used. There were several issues rendered, so this is not likely to be added back again any time soon. In order to have a more smart reactivity, we would need places to hook user callback code (like Vue or React do), but provided by Jenkins UI, or in a way that could take some time to create in the plug-in without breaking things, and that works well.
The merge request mentions JENKINS-71365 (prototype.JS is removed from Jenkins core) but reverting this should not impact that. What we are doing is removing the async calls and promises, and instead using the synchronous code again. This is so that Active Choices parameters are rendering in order as they were before. Whilst rendering them asynchronously seemed like a good idea to provide a more responsive UI, in reality we oversaw an important aspect: users write their scripts relying on the order parameters are loaded. Users wrote their code using document.getElementId, for instance, which worked previously as users would put the parameter doing that after the parameter from which the ID was being used. There were several issues rendered, so this is not likely to be added back again any time soon. In order to have a more smart reactivity, we would need places to hook user callback code (like Vue or React do), but provided by Jenkins UI, or in a way that could take some time to create in the plug-in without breaking things, and that works well.
The merge request mentions JENKINS-71365 (prototype.JS is removed from Jenkins core) but reverting this should not impact that. What we are doing is removing the async calls and promises, and instead using the synchronous code again. This is so that Active Choices parameters are rendering in order as they were before. Whilst rendering them asynchronously seemed like a good idea to provide a more responsive UI, in reality we oversaw an important aspect: users write their scripts relying on the order parameters are loaded. Users wrote their code using document.getElementId, for instance, which worked previously as users would put the parameter doing that after the parameter from which the ID was being used. There were several issues rendered, so this is not likely to be added back again any time soon. In order to have a more smart reactivity, we would need places to hook user callback code (like Vue or React do), but provided by Jenkins UI, or in a way that could take some time to create in the plug-in without breaking things, and that works well.
The merge request mentions JENKINS-71365 (prototype.JS is removed from Jenkins core) but reverting this should not impact that. What we are doing is removing the async calls and promises, and instead using the synchronous code again. This is so that Active Choices parameters are rendering in order as they were before. Whilst rendering them asynchronously seemed like a good idea to provide a more responsive UI, in reality we oversaw an important aspect: users write their scripts relying on the order parameters are loaded. Users wrote their code using document.getElementId, for instance, which worked previously as users would put the parameter doing that after the parameter from which the ID was being used. There were several issues rendered, so this is not likely to be added back again any time soon. In order to have a more smart reactivity, we would need places to hook user callback code (like Vue or React do), but provided by Jenkins UI, or in a way that could take some time to create in the plug-in without breaking things, and that works well.
The merge request mentions JENKINS-71365 (prototype.JS is removed from Jenkins core) but reverting this should not impact that. What we are doing is removing the async calls and promises, and instead using the synchronous code again. This is so that Active Choices parameters are rendering in order as they were before. Whilst rendering them asynchronously seemed like a good idea to provide a more responsive UI, in reality we oversaw an important aspect: users write their scripts relying on the order parameters are loaded. Users wrote their code using document.getElementId, for instance, which worked previously as users would put the parameter doing that after the parameter from which the ID was being used. There were several issues rendered, so this is not likely to be added back again any time soon. In order to have a more smart reactivity, we would need places to hook user callback code (like Vue or React do), but provided by Jenkins UI, or in a way that could take some time to create in the plug-in without breaking things, and that works well.
The merge request mentions JENKINS-71365 (prototype.JS is removed from Jenkins core) but reverting this should not impact that. What we are doing is removing the async calls and promises, and instead using the synchronous code again. This is so that Active Choices parameters are rendering in order as they were before. Whilst rendering them asynchronously seemed like a good idea to provide a more responsive UI, in reality we oversaw an important aspect: users write their scripts relying on the order parameters are loaded. Users wrote their code using document.getElementId, for instance, which worked previously as users would put the parameter doing that after the parameter from which the ID was being used. There were several issues rendered, so this is not likely to be added back again any time soon. In order to have a more smart reactivity, we would need places to hook user callback code (like Vue or React do), but provided by Jenkins UI, or in a way that could take some time to create in the plug-in without breaking things, and that works well.
The merge request mentions JENKINS-71365 (prototype.JS is removed from Jenkins core) but reverting this should not impact that. What we are doing is removing the async calls and promises, and instead using the synchronous code again. This is so that Active Choices parameters are rendering in order as they were before. Whilst rendering them asynchronously seemed like a good idea to provide a more responsive UI, in reality we oversaw an important aspect: users write their scripts relying on the order parameters are loaded. Users wrote their code using document.getElementId, for instance, which worked previously as users would put the parameter doing that after the parameter from which the ID was being used. There were several issues rendered, so this is not likely to be added back again any time soon. In order to have a more smart reactivity, we would need places to hook user callback code (like Vue or React do), but provided by Jenkins UI, or in a way that could take some time to create in the plug-in without breaking things, and that works well.
The merge request mentions JENKINS-71365 (prototype.JS is removed from Jenkins core) but reverting this should not impact that. What we are doing is removing the async calls and promises, and instead using the synchronous code again. This is so that Active Choices parameters are rendering in order as they were before. Whilst rendering them asynchronously seemed like a good idea to provide a more responsive UI, in reality we oversaw an important aspect: users write their scripts relying on the order parameters are loaded. Users wrote their code using document.getElementId, for instance, which worked previously as users would put the parameter doing that after the parameter from which the ID was being used. There were several issues rendered, so this is not likely to be added back again any time soon. In order to have a more smart reactivity, we would need places to hook user callback code (like Vue or React do), but provided by Jenkins UI, or in a way that could take some time to create in the plug-in without breaking things, and that works well.
The merge request mentions JENKINS-71365 (prototype.JS is removed from Jenkins core) but reverting this should not impact that. What we are doing is removing the async calls and promises, and instead using the synchronous code again. This is so that Active Choices parameters are rendering in order as they were before. Whilst rendering them asynchronously seemed like a good idea to provide a more responsive UI, in reality we oversaw an important aspect: users write their scripts relying on the order parameters are loaded. Users wrote their code using document.getElementId, for instance, which worked previously as users would put the parameter doing that after the parameter from which the ID was being used. There were several issues rendered, so this is not likely to be added back again any time soon. In order to have a more smart reactivity, we would need places to hook user callback code (like Vue or React do), but provided by Jenkins UI, or in a way that could take some time to create in the plug-in without breaking things, and that works well.
The code in
makeStaplerProxy2
usedprototype.js
to create a synchronous version of the standard proxy that comes with Jenkins Core.This code switches back to the standard proxy.
That requires making the code execute in the correct order when there were dependencies. That is achieved by using
async
/await
in the.es6
code and transpiling it down to use Promises so HtmlUnit tests continue to work.Refs: JENKINS-71365
Testing done
The existing tests pass. The UI Acceptance test was relying on an unexpected side-effect of synchronous network calls.
To account for the network calls becoming asynchronous, the tests now wait for the condition to be successful instead of a straight assert.
Submitter checklist