NERSC / jupyterlab-slurm

BSD 3-Clause "New" or "Revised" License
92 stars 23 forks source link

Replace XMLHttpRequest with fetch #22

Closed tslaton closed 5 years ago

tslaton commented 5 years ago

I updated submitRequest and setJobCompletedTasks so that they use fetch instead of XMLHttpRequest. The other changes are just trailing whitespace removals my text editor automatically made.

This was in response to https://github.com/NERSC/jupyterlab-slurm/issues/9. Currently this code is untested as I don't know how to run it. I just thought I'd get started publicly.

I think just removing XMLHttpRequest is about as simple as the work I've already done. It should be easy to use $.ajax or the jupyterlab/services makeRequest instead of fetch from here. However, I'm not completely sure what the ultimate goal is with the refactoring - do we want to move away from a dependence on JQuery or not? There's a fair amount of JQuery work going on in this code aside from the requests. Do we want to do other simplifying refactoring?

krinsman commented 5 years ago

I will try to run this later to make sure there are no breaking changes, although I doubt that there will be. Please remind me to do this tomorrow if I have not gotten around to doing so by then.

tslaton commented 5 years ago

I'm planning to put up a version using this approach after the CSSS meeting, too, for comparison.

tslaton commented 5 years ago

Here's what I think it might look like with the ServerConnection approach. I'm less confident I have this correct, and I want it to be easy for you all to look at both versions, so I'm just going to paste the code here. The only function I think needs to change on top of the PR code is submitRequest.

import {
  ServerConnection
} from '@jupyterlab/services';

[...]

private submitRequest(cmd: string, requestType: string, body: string,
                      element: JQuery = null, jobCount: any = null) {
  let settings = ServerConnection.makeSettings();
  // Prepend command with the base URL to yield the final endpoint
  let endpoint = URLExt.join(settings.baseUrl, cmd);
  let fetch_init = {
    method: requestType,
    headers: {
      // add Jupyter authorization (XRSF) token to request header
      'Authorization': 'token ' + PageConfig.getToken(),
      // prevent it from enconding as plain-text UTF-8, which is the default and screws everything up
      'Content-Type': 'application/x-www-form-urlencoded',
    },
    body: body,
  }
  ServerConnection.makeRequest(endpoint, fetch_init, settings).then(response => {
    if (response.status !== 200) {
      throw Error(response.statusText);
    }
    return response.json();
  }).then(response => {
    this.setJobCompletedTasks(response, element, jobCount);
  }).catch(error => {
    console.log(error);
  });
}
krinsman commented 5 years ago

Seems to work to me -- it's what we've wanted for a long time!

krinsman commented 5 years ago

@tslaton If you want to make a new PR using the ServerConnection approach, feel free to do so. I am not sure if it matters as much as the upgrade from JQuery to native JS though. That being said you are right that this was the approach suggested by the Jupyter developers.