devdocs icon indicating copy to clipboard operation
devdocs copied to clipboard

Changing XHR to Fetch API

Open hmdros opened this issue 3 years ago • 1 comments

@arielj, @toppa and I are submitting this PR for this issue: https://github.com/freeCodeCamp/devdocs/issues/1193

We had a few differences between XHR and Fetch, so we couldn't match functionality 100%. But we think it's fine from what we saw on the code.

  • Now we abort the request with AbortController
  • Fetch API doesn't have a onProgress callback or equivalent
  • Fetch API doesn't support sync request

We couldn't find usages of the xhr argument on callback methods. We weren't sure about the arguments.

Captura de Tela 2022-10-19 às 17 56 42

hmdros avatar Oct 19 '22 20:10 hmdros

hey @simon04 we were reviewing your comments and the code, and we have some concerns if this move from xhr to fetch is actually beneficial or not. I can describe a bit of what we found:

  • fetch has no equivalent of a progress callback, the workarounds involve adding a lot of code wrapping the fetch requests into another object like https://dev.to/tqbit/how-to-monitor-the-progress-of-a-javascript-fetch-request-and-cancel-it-on-demand-107f or https://github.com/prioe/node-fetch-progress, but we are not sure if that's the direction you want to go with this code, it will be way more complex
  • the way the abort works with fetch requests requires a lot of changes in logic because fetch does not return a request object that can be aborted, it returns a promise, so the abort callback would have to be passed as an option from the caller (it requires changing entry_page.coffee, entry.coffee, db.coffee, and ajax.coffee just to pass around an abort function)

Also, we noticed one thing that seems to be a bug in the current code, this error callback https://github.com/freeCodeCamp/devdocs/blob/14049dac0c18d1ea1ff7f439a63cf03a0d425b4d/assets/javascripts/app/update_checker.coffee#L17 is expecting the xhr as the second argument, but that's actually passed as the third argument here https://github.com/freeCodeCamp/devdocs/blob/14049dac0c18d1ea1ff7f439a63cf03a0d425b4d/assets/javascripts/lib/ajax.coffee#L97

We want to raise these concerns before continuing with this change, we are not sure if it's really worth it (the current code works just fine and some of the code around it was designed for how xhr works)

arielj avatar Oct 24 '22 14:10 arielj