fix: prevent code execution order issues around SvelteKit's env modules
#14632
Conversation
…ules With #14571 we hoped to have solved the chunk/code execution order issues, but they snuck back in. The problem, it turns out (read https://rollupjs.org/configuration-options/#output-manualchunks very carefully), is that dependencies of a manual chunk might get pulled into that manual chunk, too. If the dependency happens to be our env module, this can cause issues since the module might eagerly access the env. Luckily Rollup has recently solved this via a dedicated option (that will be the default in version 5 and is hence deprecated right away) named `onlyExplicitManualChunks` (rollup/rollup#6087). This solves our problems around chunk ordering. For users not on the latest version yet we do a best-effort fallback by extracting env into its own chunk. This should solve most issues people encounter but the general problem can still occur, which is only fixed with the new option (hence we warn if the rollup version is not up to date). Fixes #14590
🦋 Changeset detectedLatest commit: 1ef2143 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 |
|
|
||
| if (!is_outdated_rollup) { | ||
| // @ts-expect-error only exists in more recent Rollup versions https://rollupjs.org/configuration-options/#output-onlyexplicitmanualchunks | ||
| config.build.rollupOptions.onlyExplicitManualChunks = true; |
There was a problem hiding this comment.
Should this be config.build.rollupOptions.output.onlyExplicitManualChunks = true; ?
There was a problem hiding this comment.
For reference, building a project produces the following warning and then continues to produce the env errror: Unknown input options: onlyExplicitManualChunks
|
While this fixes the env bug, this seems to cause a circular dependency now, on both the latest rollup, and rolldown. What I'm seeing (and I'm not sure I can make a repro just because the library is large) is the generated I do think this is probably an extremely unlikely build for an app though. For reasons I don't want to get into, there is data in two separate databases (both being accessed by drizzle) and the svelte-kit backend is essentially zippering the two together. |
|
if files import each other could it be that the circular dependency also exist in the original source? I'm having trouble understanding how else Rollup would order them that way, since no dependencies will be inlined with the strict setting |
We tried very hard to get manualChunks to work (see #14632 for the latest attempt) but it just doesn't quite work out. What we need is to actually have Vite/Rollup know about the proper entry points to then resolve everything from there. The problem previously was that we didn't know of a good way to know which entry points we have on the fly. Turns out, `this.emitFile` is our savior (https://rollupjs.org/plugin-development/#this-emitfile). We can use it to create entry points while traversing the module graph and have Vite/Rollup wire up everything correctly. Fixes #14679
4 participants
With #14571 we hoped to have solved the chunk/code execution order issues, but they snuck back in.
The problem, it turns out (read https://rollupjs.org/configuration-options/#output-manualchunks very carefully), is that dependencies of a manual chunk might get pulled into that manual chunk, too. If the dependency happens to be our env module, this can cause issues since the module might eagerly access the env.
Luckily Rollup has recently solved this via a dedicated option (that will be the default in version 5 and is hence deprecated right away) named
onlyExplicitManualChunks(rollup/rollup#6087). This solves our problems around chunk ordering. For users not on the latest version yet we do a best-effort fallback by extracting env into its own chunk. This should solve most issues people encounter but the general problem can still occur, which is only fixed with the new option (hence we warn if the rollup version is not up to date).Fixes #14590
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.