Skip to content
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

Use fetch inside Axios in the background #2108

Merged
merged 2 commits into from
Dec 14, 2021
Merged

Use fetch inside Axios in the background #2108

merged 2 commits into from
Dec 14, 2021

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Dec 12, 2021

Extracted from:

Context:

  • Axios is based on XMLHttpRequest but that's not available in Service Workers
  • This PR adds an adapter that uses fetch instead of XMLHttpRequest, only in the background page

Warnings:

  • The adapter I found isn't well-tested nor used much, it's likely that it's not feature-complete
  • This isn't actually required until we switch to MV3, but we could still merge it just to test it
  • Not really tested, I'm just floating the idea first

The alternative to merging this is to:

@fregante
Copy link
Contributor Author

This appears to work in preliminary tests, including setting URL parameters, headers and POST body, using the HTTP Request brick.

I'd still classify this as an "experimental PR" due to the warnings I mentioned previously.

Before

Background page "Network" tab

Screen Shot 5

After

Screen Shot 4

@twschiller
Copy link
Contributor

twschiller commented Dec 13, 2021

For the background we don't use much of the axios surface area (it's just JSON payloads, base URLs, header logic).

I'd probably lean toward an adapter so most programmers only need to reason about the Axios API? I'm also OK either using that library adapter or wring our own for the functionality we need (glancing at the adapter, the code is straightforward, and actually probably covers more features than we need)

The main benefits of Axios are around the automatic JSON handling, adapters, transforms, retry logic, etc. Looking at some docs there's also slight differences on how it determines whether a response is "ok", etc. But those probably won't matter for us in practice

@fregante
Copy link
Contributor Author

fregante commented Dec 13, 2021

I'd probably lean toward an adapter so most programmers only need to reason about the Axios API?

Yeah that's useful when writing tests. The current PR only enables fetch in the background because that's where it's needed by MV3.

If this already works for you it's easier than replacing Axios with Fetch (or other fetch-based library).

Merge at will — or not, we can alternatively only use it if it's running as MV3.

@twschiller twschiller self-requested a review December 14, 2021 02:57
@twschiller
Copy link
Contributor

twschiller commented Dec 14, 2021

Merge at will — or not, we can alternatively only use it if it's running as MV3.

I'm in favor of moving toward the more modern browser API. (And definitely against conditionally using fetch)

Reading more about XHR vs. fetch, looks some slight difference in CORS and passing along cookies which shouldn't impact us

Let's use this library for now. We'd like end up forking it or inlining it if we need to make any changes. Looks like it's not really maintained

@twschiller twschiller merged commit 2141f8d into main Dec 14, 2021
@twschiller twschiller deleted the F/mv3/axios-fetch branch December 14, 2021 03:14
@twschiller twschiller added this to the 1.4.11 milestone Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants