[fix] don't polyfill undici if using Deno or Bun

[benmccann]

Fixes #8324

[changeset-bot]

🦋 Changeset detected

Latest commit: c55bdca

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

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	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

[Conduitry]

I find the version sniffing a little unpleasant. And, especially since we were already pinning to a specific Undici version before because of problems we were running into, it might potentially cause breakages in people's apps if we deferred to whatever version were built into their version of Node. What if we just checked for process.version.node and installed our polyfill if that is present (i.e., for all versions of Node) and skipped it if not (i.e., for Deno, Bun, etc.)?

[benmccann]

That could work too. I don't have strong feelings about one vs the other, so went ahead and changed it as you suggested

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
kit-svelte-dev	🔄 Building (Inspect)		Jan 5, 2023 at 6:23PM (UTC)

[jakubdonovan]

Fixes #8324

Applying the fix allowed me to start my app in dev mode with deno task dev but I got the error:

➜ deno task dev
Task dev deno run -A --node-modules-dir npm:vite

VITE v4.0.4 ready in 14491 ms

➜ Local: http://localhost:5173/
➜ Network: use --host to expose
➜ press h to show help
TypeError: Cannot read properties of undefined (reading 'body')
at handleResponse (deno:ext/flash/01_http.js:259:19)
at deno:ext/flash/01_http.js:578:25
TypeError: Cannot read properties of undefined (reading 'body')
at handleResponse (deno:ext/flash/01_http.js:259:19)
at deno:ext/flash/01_http.js:578:25

Minimal repo https://github.com/jakubdonovan/deno-sveltekit

[bartlomieju]

Hey folks, Bartek from the Deno team here. This fix won't work - when executing code from npm packages we are injecting globals like process or setImmediate behind the scenes, if we didn't do that most of packages wouldn't work as they often rely on process global. I suggest to do typeof globalThis.Deno !== "undefined" check instead

[bartlomieju]

The error reported in #8338 (comment) is caused by this check

[benmccann]

@bartlomieju thanks! I had tested process.versions.node directly and not through an npm package. I'm curious, what do you set process.versions.node to? I wonder if my earlier solution of polyfilling only when the version is less than 18.11.0 might work for Deno

[Jarred-Sumner]

I find the version sniffing a little unpleasant. And, especially since we were already pinning to a specific Undici version before because of problems we were running into, it might potentially cause breakages in people's apps if we deferred to whatever version were built into their version of Node. What if we just checked for process.version.node and installed our polyfill if that is present (i.e., for all versions of Node) and skipped it if not (i.e., for Deno, Bun, etc.)?

For Bun, either typeof globalThis.Bun !== “undefined” or typeof process.versions.bun !== “undefined”.

process.versions.node is set in Bun because many npm packages will fail to load or do something strange if it’s not set

[benmccann]

Thanks @Jarred-Sumner. I'm curious, what do you set the Node version to? Would it be something similar to Node like 20.0.2 or more like a fake value like 100.0.0 or 1.0.0 or something? I wonder if my earlier solution of polyfilling only when the version was less than 18.11.0 might work for Bun

[dummdidumm]

Sounds like we should have a blacklist (check who shouldn't get the polyfill, e.g. bun and deno) instead of a whitelist (check for node) either way.

[Rich-Harris]

Seems to me the environment detection should happen at the relevant installPolyfills() callsites (in dev/preview/prerender etc) rather than inside the function, no? We don't want this cruft in production

[benmccann]

Seems to me the environment detection should happen at the relevant installPolyfills() callsites (in dev/preview/prerender etc) rather than inside the function, no? We don't want this cruft in production

I had debated that. I did it this way to reduce the amount of code required and also because it'd probably allow adapter-node to be used by Deno and Bun instead of needing a new adapter for each environment. There are already Deno and Bun adapters, but some users may not discover them and if adapter-node just worked out of the box that'd be cool (e.g. I think the Deno folks weren't aware of adapter-deno). The runtime cost in production would be pretty minimal and it's always nice to keep dev and production working similarly

[Rich-Harris]

People using adapter-node for Deno/Bun deployments would be a terrible outcome!

As this PR stands we'd be adding the detection code to Netlify and Vercel apps (plus any community adapters that use the polyfills), quite unnecessarily. It doesn't belong inside the function