-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
create commonjs bundle with adapter-node, since esm output has issues #6855
Conversation
🦋 Changeset detectedLatest commit: 1239904 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
I have the feeling you're not going to like it, but: Would it make sense for this to be configurable? Also, what file extension are these being written out as? It looks like still Edit: Oh, never mind about the last bit, I see you're writing |
Would definitely prefer to avoid configuration if we can, just because knowing when to use which is such an in-the-weeds problem. I guess the question is whether or not there are cases where an ESM package couldn't be bundled. I suppose you could have an ESM package that depended on a native module like I guess an alternative would be some version of evanw/esbuild#1921 (comment) — e.g. rewrite all the import { createRequire } from 'module';
const require = createRequire(import.meta.url); |
I'm less worried about adding potentially regrettable APIs to adapters than I am to SK core. If we can't/don't want to do something like the Having a nasty API wart isn't as bad as telling some people they are just SOL unless they want to fork the adapter. If we find a satisfactory workaround for the esbuild problem (or if esbuild addresses this itself), then we can remove the option and release a new major version of the adapter. |
My solution, which isn't an immediate one, is to fix Vite so that we no longer need esbuild involved. I filed vitejs/vite#9919 about this and if Vite worked the way patak described then we could remove esbuild. I'm not sure if it should be considered a bug that might be able to be fixed in Vite 3 that it doesn't behave as expected, but there's a chance we can't fix it until Vite 4 a couple of months from now. Either way, I'll be pushing to get that fixed and drop esbuild so that we can have ESM and avoid all the esbuild bugs |
Alternative approach here: #6896 |
closing as #6896 was merged |
This is an alternative approach to #6797. By generating CommonJS instead of ESM,
esbuild
doesn't replacerequire
calls for built-in modules with__require
(which throws a 'Dynamic require of ... is not supported' error — evanw/esbuild#1921).As such, it's possible to bundle libraries that have such calls, instead of having to move them to
dependencies
so that they're not bundled, which is onerous since it's very hard to figure out which are the offending packages.The downside of this approach is that it's no longer possible to have ESM packages as external dependencies — they must always be bundled. I don't have a clear sense of which is the lesser of two evils.
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. All changesets should bepatch
until SvelteKit 1.0