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

[javascript] Remove fetch polyfill #1162

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arjunyel
Copy link
Contributor

Motivation

svix-fetch uses Node specific APIs which causes issues when using non-node environments, like Cloudflare Workers, and was imported as a side effect which means that bundlers cannot tree shake svix. fetch is now supported on all maintained Nodejs versions. Also as a general rule users should be responsible for polyfilling fetch, not libraries.

Solution

Use fetch from globalThis and mark the library as side effect free.

@arjunyel arjunyel force-pushed the remove-fetch-polyfill branch 2 times, most recently from 5c8e95a to 1e6ee87 Compare January 18, 2024 19:40
@svix-lucho
Copy link
Contributor

svix-lucho commented Jan 23, 2024

@arjunyel The reason why we use svix-fetch is not to polyfill fetch, it's to enable keepalives when running in a node context. I agree with the issues with non-node environments and tree-shaking, but we need to make sure that we are not losing the keepalive behavior when needed.

UPDATE: keepalive is enabled by default as of Node 19 - https://nodejs.org/en/blog/announcements/v19-release-announce#https11-keepalive-by-default. So I think we can go ahead with this for future versions.

svix-lucho
svix-lucho previously approved these changes Jan 23, 2024
@svix-lucho svix-lucho dismissed their stale review January 26, 2024 13:45

Discussed this with the team, and we decided that Node 19 is just too recent to depend on it.

@arjunyel
Copy link
Contributor Author

The end user can set keep alive on their fetch for Node 18 https://stackoverflow.com/questions/62500011/reuse-tcp-connection-with-node-fetch-in-node-js

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

Successfully merging this pull request may close these issues.

None yet

2 participants