fix: identify circular imports from src/env - #6
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: sveltejs#17014 Source head: 2c90fd5
✅ Shipwright · ApproveRecommendation: approve PR #6 · Tier
Findings (4)
Fireworks usage: 7,819 input · 462 output · 8,281 total tokens · $0.0020 · 9s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| `Cannot import \`$app/*\` modules other than \`$app/env\` inside \`src/env\``, | ||
| { cause: e } | ||
| if (error.code === 'ERR_MODULE_NOT_FOUND') { | ||
| const match = error.message?.match( |
There was a problem hiding this comment.
Shipwright · HIGH
The regex /<sveltekit:generated>/env/(private|public)/server.js/ is a magic internal path pattern with no comment explaining why this exact generated path indicates a circular
Impact: The regex /<sveltekit:generated>/env/(private|public)/server.js/ is a magic internal path pattern with no comment explaining why this exact generated path indicates a circular dependency. A future maintainer cannot tell whether the pattern is complete or will silently break if the generated path changes.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| plugins: [ | ||
| { | ||
| name: 'dependency-scanner', | ||
| enforce: 'pre', |
There was a problem hiding this comment.
Shipwright · HIGH
The new resolveId hook records the importer for $app/env/private or public, but it never clears or scopes env_importers per resolution.
Impact: The new resolveId hook records the importer for $app/env/private or public, but it never clears or scopes env_importers per resolution. If the same load_explicit_env call resolves the same virtual module from multiple importers, the map will retain only the last importer, so the eventual circular-dependency error can blame the wrong file. This makes the diagnostic misleading in multi-import scenarios.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| throw new Error( | ||
| `Cannot import \`$app/*\` modules other than \`$app/env\` inside \`src/env\``, | ||
| { cause: e } | ||
| if (error.code === 'ERR_MODULE_NOT_FOUND') { |
There was a problem hiding this comment.
Shipwright · HIGH
The error path now only special-cases ERR_MODULE_NOT_FOUND.
Impact: The error path now only special-cases ERR_MODULE_NOT_FOUND. If the circular import manifests as a different Rollup/Vite error code, the original error is rethrown without the new explanation, so users can still get the confusing failure this changeset claims to fix.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| } | ||
| }); | ||
|
|
||
| test('explains circular imports through $app/env/private', async () => { |
There was a problem hiding this comment.
Shipwright · HIGH
The new test writes a temporary env entry and helper under node_modules/.svelte-kit-env-* inside the real basics app directory.
Impact: The new test writes a temporary env entry and helper under node_modules/.svelte-kit-env-* inside the real basics app directory. If the test fails before the finally block, or if cleanup is interrupted, it leaves generated files in node_modules that could be picked up by other tooling or accidentally committed.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
Fixes sveltejs#17013.
When
src/envimports a dependency that imports$app/env/privateor$app/env/public, loading the environment configuration depends on modules that have not yet been generated.Recognize this resolution failure and report the circular dependency directly instead of exposing the internal generated-module path. When available, the diagnostic now identifies the project-relative module that imported
$app/env/privateor$app/env/public.A regression test covers the transitive helper-import pattern and verifies that the responsible helper module is reported.
Testing
pnpm vitest run -c vitest.kit.config.js src/core/sync/sync.spec.js --reporter=verbosepnpm -F @sveltejs/kit test:unitpnpm formatpnpm -F @sveltejs/kit lintpnpm -F @sveltejs/kit prepublishOnlySource merge-base:
2df68224d77558ce6a0fc21513c5cd7f299c60d1Source head:
2c90fd575e104fd3c4be91756495774e8585d906