-
Notifications
You must be signed in to change notification settings - Fork 28
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
change to using fetch for ajax instead of jquery function #764
Conversation
While they probably do, we should double-check that all of our supported browsers support these JS features: https://caniuse.com/?search=promise |
} | ||
return new Promise(function(resolve, reject) { | ||
fetch(url, options) | ||
.then(function(response) { |
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.
not that it can happen often, but the fetch promise could need a catch if network interrupted or something so may want to catch the fetch promise too:
https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
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 put in a catch and tried issuing a request with the network disconnected but nothing ever happens. We could perhaps put in a timer to detect network disconnection.
If the url has a spurious character inserted at the beginning the error message that appears is "TypeError: NetworkError when attempting to fetch resource." so it seems to be working.
If you edit the tail of the url for example api -> apix then then an "incorrect response type" message appears which is correct because the html page for "file not found" is sent.
I love the switch to fetch! |
Promise and fetch are ok for Chrome 54 Firefox 52 Microsoft Edge Opera 41 Safari 11 |
scripts/api.js
Outdated
"headers": { | ||
"X-API-KEY": "SESSION" | ||
} | ||
function ajax(method, apiUrl, queryParams = {}, data = {}) { |
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 it'd be nice if we could write a few unit tests for this. Obviously we'll need to mock fetch, so maybe a 5th param that we can set in unit tests to a Promise mock that we implement. Like:
function ajax(method, apiUrl, queryParams = {}, data = {}, fetchPromise = fetch) {
...
}
And then in the unit tests pass a mock like:
ajax('myMethod', 'myapiUrl', {}, {}, Promise.resolve({ok: true, json: () => Promise.resolve({data: {}}));
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.
Yes, I'll look at doing this.
Looks good to me now. I look forward to the unit tests though if you intend to do them. |
I started working on this but ran into a problem with an interaction with another test, getting the message "validCharacterPattern is not defined". The ajax test works if the characterValidtion test and the file it uses are removed from the unit tests. I'll look into fixing this in another branch since it is not directly related to this MR. |
I started working on this but ran into a problem with an interaction with another test, getting the message "validCharacterPattern is not defined". The ajax test works if the characterValidtion test and the file it uses are removed from the unit tests. I'll look into fixing this in another branch since it is not directly related to this MR. |
Added some tests following Chris's example now that the character test issue has been resolved and updated the sandbox. |
Browserstack testing: High Sierra:
Windows 7
Windows 10 For the initial testing, I verified scrolling through pages and resizing a bit, but even so, didn't do exhaustive functional testing; mostly verified that it looked reasonable (i.e. toolbars in place, could scroll pages, could resize window without losing vertical toolbar options. After the more extensive testing on High Sierra, I mostly verified that the max versions that threw an exception on macOS also did so on Windows, and that the min version that didn't also matched. I also verified that the oldest Edge worked okay, and totally ignored IE 😁 . I went looking for version information; I didn't look at opera, but found this for Chrome and FF: Chrome 54: 2016-10-12 Firefox 52: 2017-03-07 Opera 41: 2016-10-25 So, essentially, functionality for this was added in September of 2017 for all three major cross-platform browsers (Safari may be a major browser for Mac, iPhone and iPad users, but is not cross-platform). |
Thanks Sharon! To reframe what Sharon discovered, our currently supported browsers are:
And this would change it to:
We should see what JS feature we are using that doesn't work. The "Invalid API path" error sounds like the URL isn't being constructed correctly. According to caniuse.org template strings are supported in all of those versions though which was my first guess. |
Interesting, though, that browsers that were about 6 months newer than our currently supported set do work with the code as-is? |
That doesn't surprise me. This code is using some JS features that we haven't used before. I can't find anything obvious at caniuse.com that jumps out as the problem though. |
Looks like it is the URLSearchParams constructor. |
The URLSearchParams constructor could not accept a "record" but it's possible to append key,value pairs one at a time since FF29, chrome49, opera36. safari10.1, Edge17. Revised code and updated the sandbox. |
The previously erroring browser versions work with the new code. I didn't do as much testing, just mostly confirmed that the affected high and low end of the previously not working versions work now on one OS or another, which is hopefully good enough. |
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.
Thanks Ray!
Given the prod deployment tomorrow morning and the code release aimed for the end of the week, I'm going to hold off merging this in until after we cut the release. I'd prefer this change to bake on test before rolling it out and a bit of bake time on prod before including it in a release.
Thanks Sharon, I should have read the doc. https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/URLSearchParams more carefully instead of just looking at the top line of Browser compatibility. |
javascript native fetch uses the promise mechanism. This should make the code less incomprehensible than using two different methods for asynchronous operations.
There should not be any difference in function.
Sandbox at: https://www.pgdp.org/~rp31/c.branch/api_fetch