fix(adapter-node): fail the build on imports that resolve to no installed package - #16655
Closed
Stadly wants to merge 1 commit into
Closed
fix(adapter-node): fail the build on imports that resolve to no installed package#16655Stadly wants to merge 1 commit into
Stadly wants to merge 1 commit into
Conversation
…lled package Only `dependencies` are external, so everything else is bundled and any bare import left in the output has to resolve against a deployment that will not contain it. When it cannot, the failure is both fatal and silent: the build succeeds, the image builds, and the server exits during module evaluation with ERR_MODULE_NOT_FOUND. For `instrumentation.server.js` that happens before the server can log anything at all. A dependency that statically imports an unmet optional peer is enough to trigger it. Rolldown reports "Module not found, treating it as an external dependency" and carries on, which is easy to lose among the other build output and does not stop a broken bundle from shipping. Check what was actually emitted rather than trusting the graph: walk the written chunks' imports, skip our own chunks, relative specifiers and builtins, and resolve the rest from the project root. Anything that fails to resolve there cannot resolve at runtime either, so fail the build and name the specifier and the chunk it came from.
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/36117ba8b79499a23c074325b017e47715b17df4Open in Note This PR is from a fork. A maintainer must approve approve each commit before it can be built and installed. |
🦋 Changeset detectedLatest commit: 36117ba The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
Contributor
Author
|
Closing per the discussion in #16653: the premise doesn't hold up on this branch, since rolldown already warns when it can't resolve an import, and resolving every emitted import isn't something the adapter should take on. The root cause is rollup/plugins#2018. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #16653 (does not close it — see "Scope" below)
adapter-node marks only
pkg.dependenciesas external, so everything else is bundled and any bare import left in the output has to resolve against a deployment that will not contain it. When it cannot, the failure is fatal and silent: the build succeeds, the image builds, and the server exits during module evaluation withERR_MODULE_NOT_FOUND. Forinstrumentation.server.jsthat happens before the server can log anything at all — you get a container that starts and immediately dies.This PR checks what was actually emitted rather than trusting the graph, and fails the build with the offending specifier and the chunk it came from.
Reproduction on
version-3A dependency that statically imports an unmet optional peer is enough. With a package in
node_modulescontaining:imported from
src/instrumentation.server.js, the build succeeds and produces:Rolldown does warn —
Module not found, treating it as an external dependency— but it is one line among the rest of the build output, and it doesn't stop a bundle that cannot start from being written. With this PR the build fails instead:Scope, and what this does not fix
This came out of #16653, where adapter-node 5.5.6/5.5.7 emitted
import '@babel/preset-typescript/package.json'intobuild/instrumentation.server.jsand took down a production deployment. The root cause there is@rollup/plugin-commonjshoisting arequirefrom inside acatchclause into a top-level import — fix proposed in rollup/plugins#2018 — so this PR is not a fix for #16653. It is the guard that would have turned that incident into a build error.Worth noting the two lines behave differently here. On
main(adapter-node 5.5.7, rollup +@rollup/plugin-commonjs) that catch-clause require becomes a static import and the server cannot start. Onversion-3(rolldown) the same input compiles to a lazy__require(...)and boots fine — which is why the reproduction above uses an ESM import instead, a path that is still live here.The guard itself applies equally to both lines. I have it implemented and verified against
main/5.5.7 as well, where it catches the original@babel/preset-typescriptcase directly; happy to open that as a companion PR if you'd want the fix on the released line too.Notes on the implementation
createRequirefrom the project root, which is where the deployment'snode_moduleswill be. Anything unresolvable there is unresolvable at runtime../, so they must be filtered byfileName, not by prefix — this caused false positives onenv.jsandmanifest.js-*.jsin a first attempt), relative specifiers, and Node builtins viaisBuiltin.dynamicImportsas well as staticimports.bundle.write's result is bound tobundledrather thanwritten, sincewrittenis already taken further up.Verification
pnpm testinpackages/adapter-node: 16/16 passpnpm lint: cleanpnpm check: 7 errors, identical with and without this change (all pre-existing, from unbuilt@sveltejs/kittypes in my sparse checkout)Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
The existing adapter-node tests don't build fixture apps, so there's no harness a test for this could hang off. #16305 proposes exactly that — a fixture app built with the real adapter and booted — and this guard would slot into it naturally. Happy to add one here if you'd rather not wait for that, or to rebase on it.
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:.Edits