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

Sveltekit support for Deno in dev #8324

Closed
Jakob5358 opened this issue Jan 3, 2023 · 3 comments · Fixed by #8338
Closed

Sveltekit support for Deno in dev #8324

Jakob5358 opened this issue Jan 3, 2023 · 3 comments · Fixed by #8338
Labels
adapters - general Support for functionality general to all adapters feature request New feature or request

Comments

@Jakob5358
Copy link

Describe the problem

A couple of releases ago, deno realesed npm support, so you can use svelte + vite in deno. Sveltekit however gives an error as following when you visit the page in the browser and the page won't load.
image

As you can see below, a deno maintainer found that the issue was that sveltekit was polyfilling apis here from undici. They don't work quite the same as those apis in deno causing the error. Polyfills are injected here:

const globals = {
crypto,
fetch,
Response,
Request,
Headers,
ReadableStream,
TransformStream,
WritableStream,
FormData
};
// exported for dev/preview and node environments
export function installPolyfills() {
for (const name in globals) {
Object.defineProperty(globalThis, name, {
enumerable: true,
configurable: true,
writable: true,
value: globals[name]
});
}
}
(By commenting out these lines i was able to get sveltekit running properly)

More details are in his issue in the deno repo: denoland/deno#17248

Describe the proposed solution

My suggestion is to add some kind of option to avoid these polyfills all together. I did see #7668 which led to #7673 which led to it being reverted in #7675 . As I understand it this would not inject them when an api was already in globalThis, but what I'm instead suggesting is a manual option so that we can choose not to override the globals, probably in svelte.config.js.

It could for example be
const config = {
injectGlobals: false
}

Alternatives considered

As I understand it currently the node adapter is used for dev, perhaps there is some way to switch this out for a official deno adapter which unlike the one on npm also runs in dev but currently I don't see the need for this as this, making a whole new adapter just for changing one file.

Importance

i cannot use SvelteKit without it

Additional Information

The importance more means I cannot use sveltekit in deno without it.

@dummdidumm dummdidumm added adapters - general Support for functionality general to all adapters feature request New feature or request labels Jan 3, 2023
@benmccann
Copy link
Member

Rather than introducing new config I wonder if we could do this automatically. E.g. we could check for globalThis.Deno or check for Node with String(globalThis.fetch).includes("lazyUndici") (the latter courtesy of @dominikg)

@dominikg
Copy link
Member

dominikg commented Jan 4, 2023

The above is a more explicit check if the local fetch is undici and it's a bit brittle. a more generic node runtime check would be typeof process !== "undefined" && !!process?.versions?.node , maybe even parsing the node version to determine if the provided undici is sufficient.

if we want to detect deno at buildtime, it could be done with

import {DENO} from 'node-conditions';
if(DENO) {
  ...
}

(https://github.com/svitejs/node-conditions/tree/main/packages/node-conditions)

@benmccann
Copy link
Member

benmccann commented Jan 4, 2023

I don't know if detecting at buildtime would work. There's just one version of the SvelteKit dev server whether you're running Node or Deno. But presumably node-conditions would also work at runtime, right?

Though I also found process.versions.node, which is probably an even better way to do this:

$ node -e 'console.log(globalThis.process?.versions?.node)'
16.15.0
deno eval 'console.log(globalThis.process?.versions?.node)'
undefined

Then we'd only have to force-polyfill the versions of Node with broken versions of Undici. multipart/form-data was added in Undici 5.11, which means we need to polyfill Node versions before 18.11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters - general Support for functionality general to all adapters feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants