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

chore(remix-node): put native fetch behind config option #9198

Merged
merged 2 commits into from
Apr 4, 2024

Conversation

jacob-ebey
Copy link
Member

@jacob-ebey jacob-ebey commented Apr 4, 2024

chore(remix-serve): default to native fetch for single-fetch

Since there are some quite major differences between our, and native fetch implementations we have opted to keep our polyfill around for existing apps until the next major to avoid introducing unexpected issues for users specifically around the garbage collection behavior of response bodies.

Due to reliance on a more spec compliant headers implementation for single fetch, single fetch opts you into native fetch.

chore(remix-serve): default to native fetch for single-fetch
Copy link

changeset-bot bot commented Apr 4, 2024

🦋 Changeset detected

Latest commit: 7cb7f93

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/node Patch
@remix-run/serve Patch
@remix-run/architect Patch
@remix-run/dev Patch
@remix-run/express Patch
@remix-run/react Patch
@remix-run/testing Patch
create-remix Patch
remix Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/server-runtime Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -37,6 +38,7 @@ module.exports = function rollup() {
extensions: [".ts", ".tsx"],
}),
nodeResolve({ extensions: [".ts", ".tsx"] }),
commonjs({ ignoreDynamicRequires: true }),
Copy link
Member Author

@jacob-ebey jacob-ebey Apr 4, 2024

Choose a reason for hiding this comment

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

Don't hoist dynamic require's to to the module scope to avoid loading two polyfills at startup.

@jacob-ebey jacob-ebey marked this pull request as ready for review April 4, 2024 18:44
@@ -4,5 +4,5 @@ module.exports = {
displayName: "testing",
setupFiles: [],
testEnvironment: "jsdom",
setupFilesAfterEnv: ["@testing-library/jest-dom", "./jest.setup.js"],
setupFilesAfterEnv: ["./jest.setup.js", "@testing-library/jest-dom"],
Copy link
Member Author

Choose a reason for hiding this comment

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

The order here matters due to testing-library setting up globals badly if they don't exist.

Copy link
Contributor

@brophdawg11 brophdawg11 left a comment

Choose a reason for hiding this comment

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

Made a minor update to the changeset so it reminds me to make the right updates to the release notes

@jacob-ebey jacob-ebey merged commit 89eb3cf into release-next Apr 4, 2024
5 checks passed
@jacob-ebey jacob-ebey deleted the fetch-polyfill-native-condition branch April 4, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants