Skip to content
This repository has been archived by the owner on Oct 10, 2024. It is now read-only.

feat: update fetch configuration logic #65

Closed
wants to merge 14 commits into from

Conversation

sublimator
Copy link
Contributor

@sublimator sublimator commented Apr 29, 2024

Superceded by #66

Closes:

Update fetch detection logic (See: #63 (comment)):

  • Add client._fetch hook point for tests rather than using a global
  • Use top level await, per here and here (this won't work in commonjs)
  • Use native fetch when available on node
  • Support service workers such as manifest version 3 Web Extensions

src/client.js Outdated
if (isNode) {
if (isNode && !haveNativeFetch ||
// The test mocks do not return a body with getReader
typeof response.body.getReader === 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we can remove the condition on isNode and haveNativeFetch as the second part of the condition will catch this anyway

src/client.js Outdated

initializeFetch();
const configuredFetch = isNode && !haveNativeFetch ?
(await import('node-fetch')).default : globalThis.fetch;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we should only use the haveNativeFetch condition no? Is there a case where isNode would be false and haveNativeFetch be false as well?

Copy link
Contributor Author

@sublimator sublimator Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back on laptop :) Thanks for the prodding

const configuredFetch = globalThis.fetch ??
  (await import('node-fetch')).default;

Went with this and killed off haveNativeFetch/isNode entirely by just using feature detection, and adding a comment

// When using node-fetch or test mocks, getReader is not defined
if (typeof response.body.getReader === 'undefined') {

@sublimator
Copy link
Contributor Author

sublimator commented Apr 29, 2024 via email

src/client.js Outdated
}

initializeFetch();
const configuredFetch = globalThis.fetch ??
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work now in service workers (mv3 web ext), and in newer versions of node with fetch available natively

src/client.js Outdated

initializeFetch();
const configuredFetch = globalThis.fetch ??
(await import('node-fetch')).default;
Copy link
Contributor Author

@sublimator sublimator Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this (top level await) won't work with commonjs (which this library should ideally support in the future)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using node-fetch makes some Node.js libraries incompatible with this library
6 participants