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

breaking: require Node 18.13 or newer #11172

Merged
merged 8 commits into from
Dec 5, 2023
Merged

breaking: require Node 18.13 or newer #11172

merged 8 commits into from
Dec 5, 2023

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Dec 2, 2023

Should we remove the shouldPolyfill method and polyfill whenever it's not already on globalThis?

Closes #10527

Copy link

changeset-bot bot commented Dec 2, 2023

🦋 Changeset detected

Latest commit: dff154f

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

This PR includes changesets to release 2 packages
Name Type
@sveltejs/adapter-node Major
@sveltejs/kit Major

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

@dummdidumm
Copy link
Member

Which shouldPolyfill are you talking about?

@benmccann
Copy link
Member Author

Looks like I forgot about our naming convention and it's called should_polyfill 😆

if (should_polyfill) {

@dummdidumm
Copy link
Member

I see - how are the two related? Is this a "while we're at it.."-thing? Do Bun/Deno (the ones we check in that const) not have these? Or is this for a more general future-proof thing?

@benmccann
Copy link
Member Author

I just went through our history to see how this all came about. It looks like we tried to detect whether the APIs were available, but it didn't work because undici had the APIs available, but with some missing functionality. Now we don't have that constraint anymore, so can remove a bunch of code to simplify things like removing the polyfill option from adapter-node. I also think at one point we didn't trust the undici bundled with Node and so preferred to force polyfill the APIs, but that also won't be applicable anymore.

@dummdidumm
Copy link
Member

Ok so you're proposing to remove should_polyfill and instead do if (!(x in globalThis)) { globalThis.x = x } in the polyfill method?

@benmccann
Copy link
Member Author

Yeah. I pushed that change

@dummdidumm dummdidumm merged commit ad2db74 into version-2 Dec 5, 2023
12 of 13 checks passed
@dummdidumm dummdidumm deleted the rm-node-16 branch December 5, 2023 13:43
@github-actions github-actions bot mentioned this pull request Dec 14, 2023
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