-
-
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
Pass alias
and external
to esbuild in the Cloudflare adapters
#10521
Conversation
🦋 Changeset detectedLatest commit: 5ac6b2a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
alias
to esbuild in the Cloudflare adaptersalias
and external
to esbuild in the Cloudflare adapters
external: [ 'fs' ], | ||
alias: { | ||
fs: './fs-stub.js' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using both for the same package okay? What happens in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at their docs it's essentially pointless as alias resolution happens first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering whether I should move the samples for external
and alias
out of the top example
section. I believe if people are new to SveltKit / to the adapter, they will just copy whatever example from the docs they can find to get going. And if that example contains the external
and alias
sections like defined above, this might have unintended consequences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking more about this, external would be for either modules that are loaded at runtime or to omit bundling something that won't be needed at runtime.
Something like node:buffer
would be a good example for the first case but we have another PR to externalize all node:
prefixed modules https://github.com/sveltejs/kit/pull/10544/files .
So we just have the second case, which would be something that's only dynamically imported/required on based on some condition.
There's also a request to expose the esbuild |
If we are for this change I think we should close this in favour of #9398 to unify all adapters |
FYI: rebased onto latest master |
There's also #10544 which adds |
@dummdidumm I believe I need the |
FYI: Rebased onto master |
I'll have to check with the other maintainers whether we want to do this, #9398, or neither. If we do add the ability to use esbuild options (either the subset here or all of them), I think we need to note that it is not recommended. The better solution in your case would be to tweak the |
FYI: Rebased onto master |
I think both options have their merits. The #9398 is definitely more flexible, but brings perhaps a little more maintenance efforts down the line, as users will expect all esbuild options to be available, so they will have to be made available in the type definition as they become available in esbuild.
In an ideal world, that'd be the way to go. However, the world is not ideal, …
… as you recognize yourself. And maintaining a fork of these dependencies – or maintaining a fork of the respective SvelteKit adapters – is plenty of work, but will also introduce security issues down the line. So I believe – looking at it globally – it's the smaller cost allowing users to add workarounds for issues in potentially many other modules in this single place, than in many other places and forks. (Even more so, since esbuild already has this options and it's largely exposing the specific options. They could also just have said «go fix upstream packages», but they didn't, probably for some reason.) So I'd appreciate it if either this or #9398 would be included in a future release. |
FYI: Rebased onto master |
FYI: Rebased onto master |
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
These prevents that users copy-and-paste unnecessary options from the docs.
As of #10544, Node built-ins are supported when deploying to Cloudflare. Things like /** @type {import('vite').UserConfig} */
const config = {
plugins: [sveltekit()],
resolve: {
alias: {
fs: path.resolve('./shims/fs.js')
}
}
}; As such, I believe this issue can be closed — any objections? |
#10544 resolves node stubs issue for subset of supported node modules under This PR enables to create aliases for monkey patching and partial stubs without enforcing use of compatibility flag. I think this PR still should be merged. |
Is this something that can't be achieved with vite like Rich shared? |
@ghostdevv, you are right: Rich code works 👍 I think this PR is not necessary anymore. |
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Ideally, include a test that fails without this PR but passes with it.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:
.Some external modules rely on NodeJS modules like
fs
. I've provided #10424, only to see that it's not enough. When running the worker, it will still try to loadfs
– and since CF does not provide it, the worker won't start properly. This happens despite my code not relying on the respective code-path that requires any problematic modules. For me, this is the case with ical-generator.The
alias
option allows providing a stub implementation. In my case, I aliasfs
to myfs-stub.js
, and this does the trick:This PR currently includes the changes of #10424. I could either split the PRs entirely, or we can merge just this one and close the other, or the other way round (since most of the review happened on the other PR already).