-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add polyfill option to adapter-node #8991
Conversation
Rather than add a new option I'd check the Node version and polyfill if less than Node 18. That removes the need for the user to learn about this option and set it appropriately. Alternatively, we could just set the |
There are probably some weird reasons that people can't update to node 18 - my only concern with automatic detection would be if the user is building in a different node version than they run the code in |
Hmm. That's a good point I hadn't considered, but I bet we could do the check at runtime by inserting it here:
|
We need to be careful trying to detect anything here - when we did #7668 before it caused #7673, because the bundled version of undici didn't support FormData. I don't know what Node version added support for that - or for any of the other features we might be using now. One of the benefits of having I'd probably prefer it to be an explicit opt-in by someone using adapter-node. 'I know what I'm doing, just let me use the pre-existing globals, whatever they may be.' |
Yeah, Node 18.11 would be needed:
If we do make it an option, we should at least say in the docs to only use if if you're using Node 18.11 or newer. I'd still lean towards an automatic solution, but would be okay with either |
I added a note to the docs about the option and about Node 18.11 |
Another reason to prefer the option over automatic detection is that we can exclude the polyfill from the bundle. If it's based on detecting the version at runtime, the polyfill code still needs to be present even if it'll be unused. Perhaps once we deem 18.11+ to be used widely enough we can flip the default to |
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.This would close #7374 - I wasn't sure the best way to go about this but settled on this solution. Instead of building the polyfills into the
handle.js
before publishingadapter-node
it will build ashims.js
file whichhandle.js
imports. If thepolyfill
option isfalse
when the adapter code runs then it will empty the local copy ofshims.js
so it won't end up loading any polyfills