change to using fetch for ajax instead of jquery function #764

70ray · 2022-01-31T16:48:12Z

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

cpeel · 2022-01-31T20:28:49Z

While they probably do, we should double-check that all of our supported browsers support these JS features: https://caniuse.com/?search=promise

scripts/api.js

chrismiceli · 2022-02-01T02:27:06Z

scripts/api.js

+    }
+    return new Promise(function(resolve, reject) {
+        fetch(url, options)
+            .then(function(response) {


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

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.

scripts/page_browse.js

scripts/api.js

chrismiceli · 2022-02-01T05:47:07Z

I love the switch to fetch!

70ray · 2022-02-01T08:51:17Z

While they probably do, we should double-check that all of our supported browsers support these JS features: https://caniuse.com/?search=promise

Promise and fetch are ok for Chrome 54 Firefox 52 Microsoft Edge Opera 41 Safari 11

scripts/api.js

chrismiceli · 2022-02-02T04:03:29Z

scripts/api.js

-        "headers": {
-            "X-API-KEY": "SESSION"
-        }
+function ajax(method, apiUrl, queryParams = {}, data = {}) {


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: {}}));

Yes, I'll look at doing this.

chrismiceli · 2022-02-02T19:44:38Z

Looks good to me now. I look forward to the unit tests though if you intend to do them.

70ray · 2022-02-03T14:12:00Z

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.

70ray · 2022-02-05T19:04:44Z

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.

70ray · 2022-02-05T19:05:26Z

Added some tests following Chris's example now that the character test issue has been resolved and updated the sandbox.

srjfoo · 2022-02-06T00:10:58Z

Browserstack testing:

High Sierra:

Safari 11 looks okay
Firefox 52-53 throw an "Invalid API path" exception. FF 54 works okay.
Chrome 54,-60 throw an "Invalid API path" exception. Chrome 61, 62 okay
Opera 41-47 throw the exception, Opera 48 seems to be okay.

Windows 7

FF -- verified, same results as macOS High Sierra

Windows 10
*FF -- verified, same results as macOS High Sierra

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
Chrome 56: 2017-01-25
Chrome 60: 2017-07-25
Chrome 61: 2017-09-05

Firefox 52: 2017-03-07
Firefox 54: 2017-06-13
Firefox 56: 2017-09-28

Opera 41: 2016-10-25
Opera 43: 2017-02-07
Opera 48: 2017-09-27

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).

cpeel · 2022-02-06T01:08:21Z

Thanks Sharon!

To reframe what Sharon discovered, our currently supported browsers are:

Chrome 54
Firefox 52
Microsoft Edge
Opera 41
Safari 11

And this would change it to:

Chrome 61 <-- changed
Firefox 54 <-- changed
Microsoft Edge
Opera 48 <-- changed
Safari 11

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.

srjfoo · 2022-02-06T01:56:00Z

Thanks Sharon!

To reframe what Sharon discovered, our currently supported browsers are:
* Chrome 54

* Firefox 52

* Microsoft Edge

* Opera 41

* Safari 11
And this would change it to:
* Chrome 61 <-- changed

* Firefox 54 <-- changed

* Microsoft Edge

* Opera 48 <-- changed

* Safari 11
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?

cpeel · 2022-02-06T02:14:51Z

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.

70ray · 2022-02-06T06:03:50Z

Invalid API path

Looks like it is the URLSearchParams constructor.

70ray · 2022-02-06T09:18:07Z

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.

srjfoo · 2022-02-06T23:49:28Z

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.

cpeel

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.

SETUP/tests/jsTests/ajaxTests.js

70ray · 2022-02-07T09:43:07Z

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.

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.

cpeel requested review from cpeel, chrismiceli and srjfoo January 31, 2022 20:27

chrismiceli reviewed Feb 1, 2022

View reviewed changes

scripts/api.js Outdated Show resolved Hide resolved

chrismiceli reviewed Feb 2, 2022

View reviewed changes

70ray added 11 commits February 5, 2022 18:51

change to using fetch for ajax instead of jquery function

1e67346

test for no error message on failure

999d7e0

add test for incorrect response type and catch fetch rejection

3322db2

catch instead of using 2nd param of then().

220218b

added 'Accept': 'application/json' and method.toUpperCase()

7560865

added credetials

3125f61

use upperCaseMethod variable

172b8f3

revise url construction

32a25b0

revise code for error case

cace700

add unit tests

ac7f34d

test for sent data

32647c2

70ray force-pushed the api_fetch branch from 4eb647f to 32647c2 Compare February 5, 2022 18:55

revise construction of search parameters

89b7ffc

srjfoo approved these changes Feb 6, 2022

View reviewed changes

cpeel approved these changes Feb 6, 2022

View reviewed changes

chrismiceli reviewed Feb 7, 2022

View reviewed changes

SETUP/tests/jsTests/ajaxTests.js Show resolved Hide resolved

cpeel merged commit 443817f into DistributedProofreaders:master Feb 12, 2022

70ray deleted the api_fetch branch February 14, 2022 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change to using fetch for ajax instead of jquery function #764

change to using fetch for ajax instead of jquery function #764

70ray commented Jan 31, 2022 •

edited

Loading

cpeel commented Jan 31, 2022

chrismiceli Feb 1, 2022

70ray Feb 1, 2022 •

edited

Loading

chrismiceli commented Feb 1, 2022

70ray commented Feb 1, 2022

chrismiceli Feb 2, 2022

70ray Feb 2, 2022

chrismiceli commented Feb 2, 2022

70ray commented Feb 3, 2022

70ray commented Feb 5, 2022

70ray commented Feb 5, 2022 •

edited

Loading

srjfoo commented Feb 6, 2022

cpeel commented Feb 6, 2022

srjfoo commented Feb 6, 2022

cpeel commented Feb 6, 2022

70ray commented Feb 6, 2022

70ray commented Feb 6, 2022 •

edited

Loading

srjfoo commented Feb 6, 2022

cpeel left a comment

70ray commented Feb 7, 2022

change to using fetch for ajax instead of jquery function #764

change to using fetch for ajax instead of jquery function #764

Conversation

70ray commented Jan 31, 2022 • edited Loading

cpeel commented Jan 31, 2022

chrismiceli Feb 1, 2022

Choose a reason for hiding this comment

70ray Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

chrismiceli commented Feb 1, 2022

70ray commented Feb 1, 2022

chrismiceli Feb 2, 2022

Choose a reason for hiding this comment

70ray Feb 2, 2022

Choose a reason for hiding this comment

chrismiceli commented Feb 2, 2022

70ray commented Feb 3, 2022

70ray commented Feb 5, 2022

70ray commented Feb 5, 2022 • edited Loading

srjfoo commented Feb 6, 2022

cpeel commented Feb 6, 2022

srjfoo commented Feb 6, 2022

cpeel commented Feb 6, 2022

70ray commented Feb 6, 2022

70ray commented Feb 6, 2022 • edited Loading

srjfoo commented Feb 6, 2022

cpeel left a comment

Choose a reason for hiding this comment

70ray commented Feb 7, 2022

70ray commented Jan 31, 2022 •

edited

Loading

70ray Feb 1, 2022 •

edited

Loading

70ray commented Feb 5, 2022 •

edited

Loading

70ray commented Feb 6, 2022 •

edited

Loading