Skip to content

fix: identify circular imports from src/env #6

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.

Sign up for GitHub

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

Open
anurag6569201 wants to merge 1 commit into
base: qa/agent-sveltejs-kit/pr-06-17014/base
Choose a base branch
from
Open

fix: identify circular imports from src/env #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/calm-envs-explain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

fix: clarify circular imports from `src/env`
40 changes: 33 additions & 7 deletions packages/kit/src/core/env.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import path from 'node:path';
import * as devalue from 'devalue';
import { dedent } from './sync/utils.js';
import { get_global_name, runtime_directory } from './utils.js';
import { stackless } from '../utils/error.js';
import { resolve_entry } from '../utils/filesystem.js';
import { handle_issues, validate } from '../exports/internal/env.js';
import { get_config_aliases } from '../exports/vite/utils.js';
Expand Down Expand Up @@ -37,6 +38,8 @@ export function resolve_env_entry(config, root) {
export async function load_explicit_env(kit, file, root, mode) {
/** @type {Set<string>} */
const deps = new Set();
/** @type {Map<EnvType, string>} */
const env_importers = new Map();

if (!file) {
return { variables: null, deps };
Expand Down Expand Up @@ -64,6 +67,16 @@ export async function load_explicit_env(kit, file, root, mode) {
plugins: [
{
name: 'dependency-scanner',
enforce: 'pre',

@shipwright-agent shipwright-agent Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

resolveId(id, importer) {
const prefixes = ['$app/env/', `${runtime_directory}/app/env/`];
const prefix = prefixes.find((prefix) => id.startsWith(prefix));
const type = prefix && id.slice(prefix.length);

if (importer && (type === 'private' || type === 'public')) {
env_importers.set(type, importer);
}
},
load(id) {
deps.add(id);
}
Expand Down Expand Up @@ -96,14 +109,27 @@ export async function load_explicit_env(kit, file, root, mode) {
} catch (e) {
const error = /** @type {any} */ (e || {});

if (
error.code === 'ERR_MODULE_NOT_FOUND' &&
error.message?.includes(`Cannot find module '$app`)
) {
throw new Error(
`Cannot import \`$app/*\` modules other than \`$app/env\` inside \`src/env\``,
{ cause: e }
if (error.code === 'ERR_MODULE_NOT_FOUND') {

@shipwright-agent shipwright-agent Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const match = error.message?.match(

@shipwright-agent shipwright-agent Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/<sveltekit:generated>\/env\/(private|public)\/server\.js/
);

if (match) {
const type = /** @type {EnvType} */ (match[1]);
const importer = env_importers.get(type);
const message = importer
? `Module \`${posixify(path.relative(root, importer))}\` imports \`$app/env/${type}\`, which creates a circular dependency with \`src/env\``
: `Cannot import \`$app/env/${type}\` inside \`src/env\` or its dependencies because it creates a circular dependency`;

throw stackless(message);
}

if (error.message?.includes(`Cannot find module '$app`)) {
throw new Error(
`Cannot import \`$app/*\` modules other than \`$app/env\` inside \`src/env\``,
{ cause: e }
);
}
}

throw error;
Expand Down
29 changes: 29 additions & 0 deletions packages/kit/src/core/sync/sync.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ import os from 'node:os';
import path from 'node:path';
import { expect, test } from 'vitest';
import { process_config, validate_config } from '../config/index.js';
import { load_explicit_env } from '../env.js';
import { relative_path } from '../../utils/filesystem.js';
import { posixify } from '../../utils/os.js';
import { create, update } from './sync.js';
import create_manifest_data from './create_manifest_data/index.js';

Expand Down Expand Up @@ -36,6 +38,33 @@ test('generates client manifest imports relative to the project root', () => {
}
});

test('explains circular imports through $app/env/private', async () => {

@shipwright-agent shipwright-agent Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const root = path.resolve(import.meta.dirname, '../../../test/apps/basics');
const dir = fs.mkdtempSync(path.join(root, 'node_modules/.svelte-kit-env-'));
const entry = path.join(dir, 'env.ts');

fs.writeFileSync(
entry,
`import { defineEnvVars } from '@sveltejs/kit/env';
import './helper.js';
export const variables = defineEnvVars({ FOO: {} });
`
);
const helper = path.join(dir, 'helper.ts');
fs.writeFileSync(helper, `import '$app/env/private';\n`);

try {
const config = process_config(validate_config({}), root);

await expect(load_explicit_env(config, entry, root, 'development')).rejects.toMatchObject({
message: `Module \`${posixify(path.relative(root, helper))}\` imports \`$app/env/private\`, which creates a circular dependency with \`src/env\``,
stack: ''
});
} finally {
fs.rmSync(dir, { recursive: true, force: true });
}
});

test('requests a manifest rebuild if static analysis encounters a missing route file', () => {
const manifest_data = /** @type {import('types').ManifestData} */ ({
assets: [],
Expand Down