Skip to content

Commit

Permalink
[JENKINS-71909]: Revert #79
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
kinow committed Aug 18, 2024
1 parent a0835e9 commit 95c5fed
Show file tree
Hide file tree
Showing 8 changed files with 127 additions and 161 deletions.
3 changes: 0 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,6 @@
["@babel/preset-env", {"targets": {"node": "current"}}],
"@babel/preset-typescript",
"@babel/preset-flow"
],
"plugins": [
"babel-plugin-transform-async-to-promises"
]
},
"jest": {
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
<gitHubRepo>jenkinsci/active-choices-plugin</gitHubRepo>
<node.version>18.18.0</node.version>
<yarn.version>1.22.19</yarn.version>
<ui.loading.timeout>PT60S</ui.loading.timeout>
<ui.loading.timeout>PT300S</ui.loading.timeout>
</properties>

<scm>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
<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"/>
<script type="text/javascript">
// source, references table
var referencedParameters = Array();
Expand All @@ -11,6 +10,14 @@
referencedParameters.push("${value}");
</j:forEach>

if (window.makeStaplerProxy) {
window.__old__makeStaplerProxy = window.makeStaplerProxy;
window.makeStaplerProxy = UnoChoice.makeStaplerProxy2;
}
var cascadeChoiceParameter = <st:bind value="${it}"/>; // Create Jenkins proxy
if (window.makeStaplerProxy) {
window.makeStaplerProxy = window.__old__makeStaplerProxy;
}
UnoChoice.renderCascadeChoiceParameter('#${h.escape(paramName)}', ${it.filterable}, '${h.escape(it.getName())}', '${h.escape(it.getRandomName())}', ${it.getFilterLength()}, '${h.escape(paramName)}', referencedParameters, cascadeChoiceParameter);

</script>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@
</j:choose>
</div>
</f:entry>
<st:bind value="${it}" var="dynamicReferenceParameter"/>
<script type="text/javascript">
// source, references table
var referencedParameters = Array();
Expand All @@ -64,6 +63,14 @@
referencedParameters.push("${value}");
</j:forEach>

if (window.makeStaplerProxy) {
window.__old__makeStaplerProxy = window.makeStaplerProxy;
window.makeStaplerProxy = UnoChoice.makeStaplerProxy2;
}
var dynamicReferenceParameter = <st:bind value="${it}"/>; // Create Jenkins proxy
if (window.makeStaplerProxy) {
window.makeStaplerProxy = window.__old__makeStaplerProxy;
}
UnoChoice.renderDynamicRenderParameter('#${paramName}', '${h.escape(it.getName())}', '${h.escape(paramName)}', referencedParameters, dynamicReferenceParameter);

// update spinner id
Expand Down
144 changes: 108 additions & 36 deletions src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,11 @@ var UnoChoice = UnoChoice || ($ => {
*
* @param avoidRecursion {boolean} flag to decide whether we want to permit self-reference parameters or not
*/
CascadeParameter.prototype.update = async function(avoidRecursion) {
CascadeParameter.prototype.update = function(avoidRecursion) {
let parametersString = this.getReferencedParametersAsText(); // gets the array parameters, joined by , (e.g. a,b,c,d)
console.log(`Values retrieved from Referenced Parameters: ${parametersString}`);
// Update the CascadeChoiceParameter Map of parameters
await new Promise((resolve) => this.proxy.doUpdate(parametersString, t => resolve(t)));
this.proxy.doUpdate(parametersString);

let spinner, rootDiv;
if (this.getRandomName()) {
Expand All @@ -169,9 +169,10 @@ var UnoChoice = UnoChoice || ($ => {
// The inner function is called with the response provided by Stapler. Then we update the HTML elements.
let _self = this; // re-reference this to use within the inner function
console.log('Calling Java server code to update HTML elements...');
await new Promise((resolve) => this.proxy.getChoicesForUI(t => {
let data = t.responseObject();
console.log(`Values returned from server: ${data}`);
this.proxy.getChoicesForUI(t => {
let choices = t.responseText;
console.log(`Values returned from server: ${choices}`);
let data = JSON.parse(choices);
let newValues = data[0];
let newKeys = data[1];
let selectedElements = [];
Expand Down Expand Up @@ -311,8 +312,7 @@ var UnoChoice = UnoChoice || ($ => {
parameterElement.style.height = newValues.length > 10 ? '230px' : 'auto';
} // if (parameterElement.children.length > 0 && parameterElement.children[0].tagName === 'DIV') {
} // if (parameterElement.tagName === 'SELECT') { // } else if (parameterElement.tagName === 'DIV') {
resolve(t)
}));
});
// propagate change
// console.log('Propagating change event from ' + this.getParameterName());
// let e1 = $.Event('change', {parameterName: this.getParameterName()});
Expand All @@ -323,7 +323,7 @@ var UnoChoice = UnoChoice || ($ => {
let other = cascadeParameters[i];
if (this.referencesMe(other)) {
console.log(`Updating ${other.getParameterName()} from ${this.getParameterName()}`);
await other.update(true);
other.update(true);
}
}
}
Expand Down Expand Up @@ -386,9 +386,9 @@ var UnoChoice = UnoChoice || ($ => {
//_self.cascadeParameter.loading(true);
$(".behavior-loading").show();
// start updating in separate async function so browser will be able to repaint and show 'loading' animation , see JENKINS-34487
setTimeout(async () => {
await _self.cascadeParameter.update(false);
$(".behavior-loading").hide();
setTimeout(() => {
_self.cascadeParameter.update(false);
$(".behavior-loading").hide();
}, 0);
}
});
Expand Down Expand Up @@ -437,11 +437,11 @@ var UnoChoice = UnoChoice || ($ => {
*
* @param avoidRecursion {boolean} flag to decide whether we want to permit self-reference parameters or not
*/
DynamicReferenceParameter.prototype.update = async function(avoidRecursion) {
DynamicReferenceParameter.prototype.update = function(avoidRecursion) {
let parametersString = this.getReferencedParametersAsText(); // gets the array parameters, joined by , (e.g. a,b,c,d)
console.log(`Values retrieved from Referenced Parameters: ${parametersString}`);
// Update the Map of parameters
await new Promise((resolve) => this.proxy.doUpdate(parametersString, t => resolve(t)));
this.proxy.doUpdate(parametersString);
let parameterElement = this.getParameterElement();

let spinner, rootDiv;
Expand All @@ -462,44 +462,43 @@ var UnoChoice = UnoChoice || ($ => {
// or maybe call a string to put as value in a INPUT.
if (parameterElement.tagName === 'OL') { // handle OL's
console.log('Calling Java server code to update HTML elements...');
await new Promise((resolve) => this.proxy.getChoicesForUI(t => {
this.proxy.getChoicesForUI(t => {
$(parameterElement).empty(); // remove all children elements
let data = t.responseObject();
console.log(`Values returned from server: ${data}`);
let choices = t.responseText;
console.log(`Values returned from server: ${choices}`);
let data = JSON.parse(choices);
let newValues = data[0];
// let newKeys = data[1];
for (let i = 0; i < newValues.length; ++i) {
let li = document.createElement('li');
li.innerHTML = newValues[i];
parameterElement.appendChild(li); // append new elements
}
resolve(t)
}));
});
} else if (parameterElement.tagName === 'UL') { // handle OL's
$(parameterElement).empty(); // remove all children elements
console.log('Calling Java server code to update HTML elements...');
await new Promise(resolve => this.proxy.getChoicesForUI(t => {
let data = t.responseObject();
console.log(`Values returned from server: ${data}`);
this.proxy.getChoicesForUI(t => {
let choices = t.responseText;
console.log(`Values returned from server: ${choices}`);
let data = JSON.parse(choices);
let newValues = data[0];
// let newKeys = data[1];
for (let i = 0; i < newValues.length; ++i) {
let li = document.createElement('li');
li.innerHTML = newValues[i];
parameterElement.appendChild(li); // append new elements
}
resolve(t)
}));
});
} else if (parameterElement.id.indexOf('inputElement_') > -1) { // handle input text boxes
await new Promise(resolve => this.proxy.getChoicesAsStringForUI(t => {
parameterElement.value = t.responseObject();
resolve(t)
}));
this.proxy.getChoicesAsStringForUI(t => {
parameterElement.value = t.responseText;
});
} else if (parameterElement.id.indexOf('formattedHtml_') > -1) { // handle formatted HTML
await new Promise(resolve => this.proxy.getChoicesAsStringForUI(t => {
parameterElement.innerHTML = t.responseObject();
resolve(t)
}));
this.proxy.getChoicesAsStringForUI(t => {
let options = t.responseText;
parameterElement.innerHTML = JSON.parse(options);
});
}
// propagate change
// console.log('Propagating change event from ' + this.getParameterName());
Expand All @@ -511,7 +510,7 @@ var UnoChoice = UnoChoice || ($ => {
let other = cascadeParameters[i];
if (this.referencesMe(other)) {
console.log(`Updating ${other.getParameterName()} from ${this.getParameterName()}`);
await other.update(true);
other.update(true);
}
}
}
Expand Down Expand Up @@ -795,6 +794,78 @@ var UnoChoice = UnoChoice || ($ => {
return value;
}

// Hacks in Jenkins core
/**
* <p>This function is the same as makeStaplerProxy available in Jenkins core, but executes calls
* <strong>synchronously</strong>. Since many parameters must be filled only after other parameters have been
* updated, calling Jenkins methods asynchronously causes several unpredictable errors.</p>
*
* @param url {string} The URL
* @param crumb {string} The crumb
* @param methods {Array<string>} The methods
*/
function makeStaplerProxy2(url, crumb, methods) {
if (url.substring(url.length - 1) !== '/') url+='/';
let proxy = {};
var stringify;
if (Object.toJSON) // needs to use Prototype.js if it's present. See commit comment for discussion
stringify = Object.toJSON; // from prototype
else if (typeof(JSON)=="object" && JSON.stringify)
stringify = JSON.stringify; // standard
let genMethod = methodName => {
proxy[methodName] = function() {
let args = arguments;
// the final argument can be a callback that receives the return value
let callback = (() => {
if (args.length === 0) return null;
let tail = args[args.length-1];
return (typeof(tail)=='function') ? tail : null;
})();
// 'arguments' is not an array so we convert it into an array
let a = [];
for (let i=0; i<args.length-(callback!=null?1:0); i++)
a.push(args[i]);
if(window.jQuery3 === window.$) { //Is jQuery the active framework?
$.ajax({
type: "POST",
url: url+methodName,
data: stringify(a),
contentType: 'application/x-stapler-method-invocation;charset=UTF-8',
headers: {'Crumb':crumb},
dataType: "json",
async: "false", // Here's the juice
success: function(data, textStatus, jqXHR) {
if (callback!==null) {
let t = {};
t.responseObject = () => data;
callback(t);
}
}
});
} else { //Assume prototype should work
new Ajax.Request(url+methodName, {
method: 'post',
requestHeaders: {'Content-type':'application/x-stapler-method-invocation;charset=UTF-8','Crumb':crumb},
postBody: stringify(a),
asynchronous: false, // and here
onSuccess: function(t) {
if (callback!==null) {
t.responseObject = function() {
return eval(`(${this.responseText})`);
};
callback(t);
}
}
});
}
}
};
for(let mi = 0; mi < methods.length; mi++) {
genMethod(methods[mi]);
}
return proxy;
}

function renderChoiceParameter(paramName, filterLength) {
let parentDiv = $(`#${paramName}`);
let parameterHtmlElement = parentDiv.find('DIV');
Expand All @@ -814,7 +885,7 @@ var UnoChoice = UnoChoice || ($ => {
}
}

async function renderCascadeChoiceParameter(parentDivRef, filterable, name, randomName, filterLength, paramName, referencedParameters, cascadeChoiceParameter) {
function renderCascadeChoiceParameter(parentDivRef, filterable, name, randomName, filterLength, paramName, referencedParameters, cascadeChoiceParameter) {
// find the cascade parameter element
let parentDiv = jQuery(parentDivRef);
let parameterHtmlElement = parentDiv.find('DIV');
Expand Down Expand Up @@ -874,13 +945,13 @@ var UnoChoice = UnoChoice || ($ => {

// call update methods in Java passing the HTML values
console.log('Updating cascade of parameter [', name, '] ...');
await cascadeParameter.update(false);
cascadeParameter.update(false);
} else {
console.log('Parameter error: Missing parameter [', paramName, '] HTML element!');
}
}

async function renderDynamicRenderParameter(parentDivRef, name, paramName, referencedParameters, dynamicReferenceParameter) {
function renderDynamicRenderParameter(parentDivRef, name, paramName, referencedParameters, dynamicReferenceParameter) {
// find the cascade parameter element
let parentDiv = jQuery(parentDivRef);
// if the parameter class has been set to hidden, then we hide it now
Expand Down Expand Up @@ -957,7 +1028,7 @@ var UnoChoice = UnoChoice || ($ => {

// call update methods in Java passing the HTML values
console.log('Updating cascade of parameter [', name, '] ...');
await dynamicParameter.update(false);
dynamicParameter.update(false);
} else {
console.log('Parameter error: Missing parameter [', paramName,'] HTML element!');
}
Expand All @@ -970,6 +1041,7 @@ var UnoChoice = UnoChoice || ($ => {
instance.DynamicReferenceParameter = DynamicReferenceParameter;
instance.ReferencedParameter = ReferencedParameter;
instance.FilterElement = FilterElement;
instance.makeStaplerProxy2 = makeStaplerProxy2;
instance.cascadeParameters = cascadeParameters;
instance.renderChoiceParameter = renderChoiceParameter;
instance.renderCascadeChoiceParameter = renderCascadeChoiceParameter;
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/biouno/unochoice/UiAcceptanceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@

public class UiAcceptanceTest {

private static final Duration MAX_WAIT = Duration.parse(System.getProperty("ui.loading.timeout", "PT60S"));
private static final Duration MAX_WAIT = Duration.parse(System.getProperty("ui.loading.timeout", "PT300S"));

@Rule
public JenkinsRule j = new JenkinsRule();
Expand Down
1 change: 0 additions & 1 deletion webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ module.exports = (env, argv) => ({
module: {
rules: [
{use: "ts-loader", test: /\.ts$/},
{use: "babel-loader", test: /\.es6$/},
],
},
externals: {
Expand Down
Loading

0 comments on commit 95c5fed

Please sign in to comment.