Remove Stapler dependency on Prototype or jQuery#452
Conversation
basil
changed the title
| // bind tag takes care of the dependency as an adjunct | ||
|
|
||
| function makeStaplerProxy(url,crumb,methods) { | ||
| function makeStaplerProxy(url,staplerCrumb,methods) { |
There was a problem hiding this comment.
Renaming this to avoid a clash with the Jenkins global variable named crumb, which is needed to set the "Jenkins-Crumb" header later below (not to be confused with the "Crumb" header, which is what this second argument is for…).
| for (var i=0; i<args.length-(callback!=null?1:0); i++) | ||
| a.push(args[i]); | ||
|
|
||
| if(window.jQuery === window.$) { //Is jQuery the active framework? |
There was a problem hiding this comment.
Let it be said that this dead jQuery code path would have never worked with Jenkins. progressiveRendering.jelly expects a JSON response and invokes callback.responseObject() to get the JSON. renderOnDemand.jelly expects HTML and invokes callback.responseText to get the HTML. This dead jQuery code path was only defining responseObject, so it would have worked with progressiveRendering.jelly but not renderOnDemand.jelly.
| // If running in Jenkins, add Jenkins-Crumb header. | ||
| if (typeof crumb !== 'undefined') { | ||
| headers = crumb.wrap(headers); | ||
| } |
There was a problem hiding this comment.
In the Prototype version, this was done deep inside of the guts of Ajax.Request, which Kohsuke had patched to automatically set this header (this was only the case for Jenkins' Prototype, not the Prototype in this repository!). Now that we aren't using Jenkins' patched Prototype, we have to do this ourselves. This is a leaky abstraction, but it is not the only place in Stapler where we have Jenkins-specific logic.
| fetch(url + methodName, { | ||
| method: 'POST', | ||
| headers: headers, | ||
| body: stringify(a), |
There was a problem hiding this comment.
Note: JSON.stringify cannot be used here, because it blows up when Prototype is present (which is not the case in this repository's test suite, but is the case in core's test suite and core's production environment). Fortunately Kohsuke did provide a custom stringify method for use with the dead jQuery code path, which we can use here and which solves the problem.
| }) | ||
| .then(function(response) { | ||
| if (response.ok) { | ||
| if (response.headers.has('content-type') && response.headers.get('content-type').startsWith('application/json')) { |
There was a problem hiding this comment.
The next few lines are a bit tricky, so let me break it down. progressiveRendering.jelly expects a JSON response and invokes callback.responseObject() to get the JSON. renderOnDemand.jelly expects HTML and invokes callback.responseText to get the HTML. So there are two supported use cases. This repository's tests only cover the JSON use case, so it is unclear whether the HTML usage in core was originally intended or if that usage happened to come about by exploiting the existing implementation. Nevertheless, that use case now exists, so it must continue to be supported.
The old code got the t object from Prototype, which had a responseText field. This was used for the HTML use case previously. To accommodate the JSON use case, that responseText was put in parentheses and eval'd (!) to define the responseObject() function. So in the old code, the object passed to the callback had both responseText and responseObject() defined. If you expected JSON and called responseText you'd get garbage, and if you expected text and called responseObject() you'd also get garbage. (Nobody actually did this.)
The new code is a lot cleaner and differentiates between the two use cases by checking the response header. If it's JSON, we parse the JSON with the Fetch API (not eval!) and return an object with only a responseObject() function. If it's text, we return an object with only responseText. Simple and straightforward.
| w.println("<html><body><script src='script'></script>"); | ||
| w.println("<script>var v = "+ WebApp.getCurrent().boundObjectTable.bind(this).getProxyScript()+";</script>"); | ||
| w.println("<script>var callback = function(t){var x=t.responseObject(); alert(Object.toJSON(x)); };</script>"); | ||
| w.println("<script>var callback = function(t){var x=t.responseObject(); alert(JSON.stringify(x)); };</script>"); |
There was a problem hiding this comment.
Since this test suite doesn't have Prototype present, it's OK to call JSON.stringify() here. (The test suite in core, and core itself in production, does have Prototype present, though.)
| /** | ||
| * Get the {@link WebClient} preconfigured for testing. | ||
| */ | ||
| protected WebClient createWebClient() { | ||
| WebClient webClient = new WebClient(); | ||
| webClient.getOptions().setFetchPolyfillEnabled(true); | ||
| return webClient; | ||
| } |
There was a problem hiding this comment.
Needed for HtmlUnit to be able to invoke the Fetch API.
| /** | ||
| * Blocks until the ENTER key is hit. | ||
| * This is useful during debugging a test so that one can inspect the state through the web browser. | ||
| */ | ||
| protected void interactiveBreak() throws IOException { | ||
| System.out.println("Jetty is running at " + url); | ||
| new BufferedReader(new InputStreamReader(System.in, Charset.defaultCharset())).readLine(); | ||
| } |
There was a problem hiding this comment.
I discovered a need for this while debugging.
| * {@code JSON.stringify()}, which isn't present in older browser, so we | ||
| * will add it. | ||
| */ | ||
| private void ensureDependencies(XMLOutput out) throws JellyTagException { |
There was a problem hiding this comment.
There are no dependencies left to ensure.
| } | ||
|
|
||
| public void jsFoo(String arg) { | ||
| public void jsFoo(String arg, int arg2) { |
There was a problem hiding this comment.
This test was copied to a jQuery version, which was later enhanced by supporting an extra argument (probably to test some additional use case). I am now deleting the jQuery version of this test, but I thought I'd keep this test enhancement and backport it to the original test, since if someone thought it worthwhile to enhance the other test, that same logic probably applies here as well.
Pre-release for jenkinsci/stapler#452
|
Causes JENKINS-71236. |
2 participants
This PR eliminates Stapler's dependencies on any JavaScript frameworks in favor of the Fetch API. Previously it supported either jQuery or Prototype, although the jQuery code path was dead code and would never have worked with Jenkins anyway. This PR rips out all this code in favor of native JavaScript. Core will require a very minor adaptation to deal with this change; see jenkinsci/jenkins#7805.
To test this I ran this repository's test and core's tests. I also did manual interactive testing of the Configure Project page and the Build History page, which exercised both of the use cases of
bind.js: the JSON use case and the HTML use case. In each use case I stepped through the code before and after in the debugger to verify it was working the same way with this PR. More detail in the self-review. Once I get an incremental I plan on running this through BOM as well.