diff --git a/.changeset/soft-gorillas-hear.md b/.changeset/soft-gorillas-hear.md new file mode 100644 index 000000000000..6b3d8affef90 --- /dev/null +++ b/.changeset/soft-gorillas-hear.md @@ -0,0 +1,5 @@ +--- +"@sveltejs/kit": patch +--- + +prevent loading of illegal modules in the browser, rather than during SSR diff --git a/packages/kit/scripts/special-types/$env+dynamic+private.md b/packages/kit/scripts/special-types/$env+dynamic+private.md index b4b63a9df45d..689e8e879202 100644 --- a/packages/kit/scripts/special-types/$env+dynamic+private.md +++ b/packages/kit/scripts/special-types/$env+dynamic+private.md @@ -1,6 +1,6 @@ This module provides access to runtime environment variables, as defined by the platform you're running on. For example if you're using [`adapter-node`](https://github.com/sveltejs/kit/tree/master/packages/adapter-node) (or running [`vite preview`](https://kit.svelte.dev/docs/cli)), this is equivalent to `process.env`. This module only includes variables that _do not_ begin with [`config.kit.env.publicPrefix`](https://kit.svelte.dev/docs/configuration#env). -This module cannot be imported into public-facing code. +This module cannot be imported into client-side code. ```ts import { env } from '$env/dynamic/private'; diff --git a/packages/kit/scripts/special-types/$env+static+private.md b/packages/kit/scripts/special-types/$env+static+private.md index cf2454a30596..536ce64de9c0 100644 --- a/packages/kit/scripts/special-types/$env+static+private.md +++ b/packages/kit/scripts/special-types/$env+static+private.md @@ -1,4 +1,4 @@ -Environment variables [loaded by Vite](https://vitejs.dev/guide/env-and-mode.html#env-files) from `.env` files and `process.env`. Like [`$env/dynamic/private`](https://kit.svelte.dev/docs/modules#$env-dynamic-private), this module cannot be imported into public-facing code. This module only includes variables that _do not_ begin with [`config.kit.env.publicPrefix`](https://kit.svelte.dev/docs/configuration#env). +Environment variables [loaded by Vite](https://vitejs.dev/guide/env-and-mode.html#env-files) from `.env` files and `process.env`. Like [`$env/dynamic/private`](https://kit.svelte.dev/docs/modules#$env-dynamic-private), this module cannot be imported into client-side code. This module only includes variables that _do not_ begin with [`config.kit.env.publicPrefix`](https://kit.svelte.dev/docs/configuration#env). _Unlike_ [`$env/dynamic/private`](https://kit.svelte.dev/docs/modules#$env-dynamic-private), the values exported from this module are statically injected into your bundle at build time, enabling optimisations like dead code elimination. diff --git a/packages/kit/scripts/special-types/$lib.md b/packages/kit/scripts/special-types/$lib.md index 444a39cab62c..56a1d7489e80 100644 --- a/packages/kit/scripts/special-types/$lib.md +++ b/packages/kit/scripts/special-types/$lib.md @@ -2,4 +2,4 @@ This is a simple alias to `src/lib`, or whatever directory is specified as [`con #### `$lib/server` -A subdirectory of `$lib`. SvelteKit will prevent you from importing any modules in `$lib/server` into public-facing code. See [server-only modules](/docs/server-only-modules). +A subdirectory of `$lib`. SvelteKit will prevent you from importing any modules in `$lib/server` into client-side code. See [server-only modules](/docs/server-only-modules). diff --git a/packages/kit/src/exports/vite/dev/index.js b/packages/kit/src/exports/vite/dev/index.js index 938acd639a74..a00afc67cf66 100644 --- a/packages/kit/src/exports/vite/dev/index.js +++ b/packages/kit/src/exports/vite/dev/index.js @@ -11,9 +11,7 @@ import { load_error_page, load_template } from '../../../core/config/index.js'; import { SVELTE_KIT_ASSETS } from '../../../constants.js'; import * as sync from '../../../core/sync/sync.js'; import { get_mime_lookup, runtime_base, runtime_prefix } from '../../../core/utils.js'; -import { prevent_illegal_vite_imports } from '../graph_analysis/index.js'; import { compact } from '../../../utils/array.js'; -import { normalizePath } from 'vite'; // Vite doesn't expose this so we just copy the list for now // https://github.com/vitejs/vite/blob/3edd1af56e980aef56641a5a51cf2932bb580d41/packages/vite/src/node/plugins/css.ts#L96 @@ -43,8 +41,6 @@ export async function dev(vite, vite_config, svelte_config) { /** @type {import('types').SSRManifest} */ let manifest; - const extensions = [...svelte_config.extensions, ...svelte_config.kit.moduleExtensions]; - /** @param {string} id */ async function resolve(id) { const url = id.startsWith('..') ? `/@fs${path.posix.resolve(id)}` : `/${id}`; @@ -94,12 +90,6 @@ export async function dev(vite, vite_config, svelte_config) { module_nodes.push(module_node); result.file = url.endsWith('.svelte') ? url : url + '?import'; // TODO what is this for? - prevent_illegal_vite_imports( - module_node, - normalizePath(svelte_config.kit.files.lib), - extensions - ); - return module.default; }; } @@ -110,12 +100,6 @@ export async function dev(vite, vite_config, svelte_config) { module_nodes.push(module_node); result.shared = module; - - prevent_illegal_vite_imports( - module_node, - normalizePath(svelte_config.kit.files.lib), - extensions - ); } if (node.server) { diff --git a/packages/kit/src/exports/vite/graph_analysis/index.js b/packages/kit/src/exports/vite/graph_analysis/index.js index 6c40503268c1..f74376203892 100644 --- a/packages/kit/src/exports/vite/graph_analysis/index.js +++ b/packages/kit/src/exports/vite/graph_analysis/index.js @@ -1,277 +1,107 @@ import path from 'path'; -import { normalizePath } from 'vite'; -import { remove_query_from_id, get_module_types } from './utils.js'; +import { posixify } from '../../../utils/filesystem.js'; -/** @typedef {import('./types').ImportGraph} ImportGraph */ - -const CWD_ID = normalizePath(process.cwd()); -const NODE_MODULES_ID = normalizePath(path.resolve(process.cwd(), 'node_modules')); -const ILLEGAL_IMPORTS = new Set([ - '/@id/__x00__$env/dynamic/private', //dev - '\0$env/dynamic/private', // prod - '/@id/__x00__$env/static/private', // dev - '\0$env/static/private' // prod -]); +const ILLEGAL_IMPORTS = new Set(['\0$env/dynamic/private', '\0$env/static/private']); const ILLEGAL_MODULE_NAME_PATTERN = /.*\.server\..+/; -export class IllegalModuleGuard { - /** @type {string} */ - #lib_dir; - - /** @type {string} */ - #server_dir; - - /** @type {Array} */ - #chain = []; - - /** - * @param {string} lib_dir - */ - constructor(lib_dir) { - this.#lib_dir = normalizePath(lib_dir); - this.#server_dir = normalizePath(path.resolve(lib_dir, 'server')); - } +/** + * Checks if given id imports a module that is not allowed to be imported into client-side code. + * @param {string} id + * @param {{ + * cwd: string; + * node_modules: string; + * server: string; + * }} dirs + */ +export function is_illegal(id, dirs) { + if (ILLEGAL_IMPORTS.has(id)) return true; + if (!id.startsWith(dirs.cwd) || id.startsWith(dirs.node_modules)) return false; + return ILLEGAL_MODULE_NAME_PATTERN.test(path.basename(id)) || id.startsWith(dirs.server); +} - /** - * Assert that a node imports no illegal modules. - * @param {ImportGraph} node - * @returns {void} - */ - assert_legal(node) { - this.#chain.push(node); - for (const child of node.children) { - if (this.#is_illegal(child.id)) { - this.#chain.push(child); - const error = this.#format_illegal_import_chain(this.#chain); - this.#chain = []; // Reset the chain in case we want to reuse this guard - throw new Error(error); - } - this.assert_legal(child); - } - this.#chain.pop(); - } +/** + * Creates a guard that checks that no id imports a module that is not allowed to be imported into client-side code. + * @param {import('rollup').PluginContext} context + * @param {{ cwd: string, lib: string }} paths + */ +export function module_guard(context, { cwd, lib }) { + /** @type {Set} */ + const seen = new Set(); - /** - * `true` if the provided ID represents a server-only module, else `false`. - * @param {string} module_id - * @returns {boolean} - */ - #is_illegal(module_id) { - if (this.#is_kit_illegal(module_id) || this.#is_user_illegal(module_id)) return true; - return false; - } + const dirs = { + // ids will be posixified, so we need to posixify these, too + cwd: posixify(cwd), + node_modules: posixify(path.join(cwd, 'node_modules')), + server: posixify(path.join(lib, 'server')) + }; /** - * `true` if the provided ID represents a Kit-defined server-only module, else `false`. - * @param {string} module_id - * @returns {boolean} + * @param {string} id + * @param {Array<{ id: string, dynamic: boolean }>} chain */ - #is_kit_illegal(module_id) { - return ILLEGAL_IMPORTS.has(module_id); - } + function follow(id, chain) { + if (seen.has(id)) return; + seen.add(id); - /** - * `true` if the provided ID represents a user-defined server-only module, else `false`. - * @param {string} module_id - * @returns {boolean} - */ - #is_user_illegal(module_id) { - if (module_id.startsWith(this.#server_dir)) return true; + if (is_illegal(id, dirs)) { + chain.shift(); // discard the entry point + id = normalize_id(id, lib, cwd); - // files outside the project root are ignored - if (!module_id.startsWith(CWD_ID)) return false; + const pyramid = + chain.map(({ id, dynamic }, i) => { + id = normalize_id(id, lib, cwd); - // so are files inside node_modules - if (module_id.startsWith(NODE_MODULES_ID)) return false; + return `${repeat(' ', i * 2)}- ${id} ${dynamic ? 'dynamically imports' : 'imports'}\n`; + }) + `${repeat(' ', chain.length)}- ${id}`; - return ILLEGAL_MODULE_NAME_PATTERN.test(path.basename(module_id)); - } + const message = `Cannot import ${id} into client-side code:\n${pyramid}`; - /** - * @param {string} str - * @param {number} times - */ - #repeat(str, times) { - return new Array(times + 1).join(str); - } + throw new Error(message); + } - /** - * Create a formatted error for an illegal import. - * @param {Array} stack - */ - #format_illegal_import_chain(stack) { - const dev_virtual_prefix = '/@id/__x00__'; - const prod_virtual_prefix = '\0'; + const module = context.getModuleInfo(id); - stack = stack.map((graph) => { - if (graph.id.startsWith(dev_virtual_prefix)) { - return { ...graph, id: graph.id.replace(dev_virtual_prefix, '') }; - } - if (graph.id.startsWith(prod_virtual_prefix)) { - return { ...graph, id: graph.id.replace(prod_virtual_prefix, '') }; - } - if (graph.id.startsWith(this.#lib_dir)) { - return { ...graph, id: graph.id.replace(this.#lib_dir, '$lib') }; + if (module) { + for (const child of module.importedIds) { + follow(child, [...chain, { id, dynamic: false }]); } - return { ...graph, id: path.relative(process.cwd(), graph.id) }; - }); - - const pyramid = stack - .map( - (file, i) => - `${this.#repeat(' ', i * 2)}- ${file.id} ${ - file.dynamic ? '(imported by parent dynamically)' : '' - }` - ) - .join('\n'); - - return `Cannot import ${stack.at(-1)?.id} into public-facing code:\n${pyramid}`; - } -} - -/** @implements {ImportGraph} */ -export class RollupImportGraph { - /** @type {(id: string) => import('rollup').ModuleInfo | null} */ - #node_getter; - - /** @type {import('rollup').ModuleInfo} */ - #module_info; - - /** @type {string} */ - id; - - /** @type {boolean} */ - dynamic; - - /** @type {Set} */ - #seen; - - /** - * @param {(id: string) => import('rollup').ModuleInfo | null} node_getter - * @param {import('rollup').ModuleInfo} node - */ - constructor(node_getter, node) { - this.#node_getter = node_getter; - this.#module_info = node; - this.id = remove_query_from_id(normalizePath(node.id)); - this.dynamic = false; - this.#seen = new Set(); - } - - /** - * @param {(id: string) => import('rollup').ModuleInfo | null} node_getter - * @param {import('rollup').ModuleInfo} node - * @param {boolean} dynamic - * @param {Set} seen; - * @returns {RollupImportGraph} - */ - static #new_internal(node_getter, node, dynamic, seen) { - const instance = new RollupImportGraph(node_getter, node); - instance.dynamic = dynamic; - instance.#seen = seen; - return instance; - } - - get children() { - return this.#children(); + for (const child of module.dynamicallyImportedIds) { + follow(child, [...chain, { id, dynamic: true }]); + } + } } - *#children() { - if (this.#seen.has(this.id)) return; - this.#seen.add(this.id); - for (const id of this.#module_info.importedIds) { - const child = this.#node_getter(id); - if (child === null) return; - yield RollupImportGraph.#new_internal(this.#node_getter, child, false, this.#seen); - } - for (const id of this.#module_info.dynamicallyImportedIds) { - const child = this.#node_getter(id); - if (child === null) return; - yield RollupImportGraph.#new_internal(this.#node_getter, child, true, this.#seen); + return { + /** @param {string} id should be posixified */ + check: (id) => { + follow(id, []); } - } + }; } -/** @implements {ImportGraph} */ -export class ViteImportGraph { - /** @type {Set} */ - #module_types; - - /** @type {import('vite').ModuleNode} */ - #module_info; - - /** @type {string} */ - id; - - /** @type {Set} */ - #seen; - - /** - * @param {Set} module_types Module types to analyze, eg '.js', '.ts', etc. - * @param {import('vite').ModuleNode} node - */ - constructor(module_types, node) { - this.#module_types = module_types; - this.#module_info = node; - this.id = remove_query_from_id(normalizePath(node.id ?? '')); - this.#seen = new Set(); - } - - /** - * @param {Set} module_types Module types to analyze, eg '.js', '.ts', etc. - * @param {import('vite').ModuleNode} node - * @param {Set} seen - * @returns {ViteImportGraph} - */ - static #new_internal(module_types, node, seen) { - const instance = new ViteImportGraph(module_types, node); - instance.#seen = seen; - return instance; - } - - get dynamic() { - return false; +/** + * Removes cwd/lib path from the start of the id + * @param {string} id + * @param {string} lib + * @param {string} cwd + */ +export function normalize_id(id, lib, cwd) { + if (id.startsWith(lib)) { + id = id.replace(lib, '$lib'); } - get children() { - return this.#children(); + if (id.startsWith(cwd)) { + id = path.relative(cwd, id); } - *#children() { - if (this.#seen.has(this.id)) return; - this.#seen.add(this.id); - for (const child of this.#module_info.importedModules) { - if (!this.#module_types.has(path.extname(this.id))) { - continue; - } - yield ViteImportGraph.#new_internal(this.#module_types, child, this.#seen); - } - } -} - -/** - * Throw an error if a private module is imported from a client-side node. - * @param {(id: string) => import('rollup').ModuleInfo | null} node_getter - * @param {import('rollup').ModuleInfo} node - * @param {string} lib_dir - * @returns {void} - */ -export function prevent_illegal_rollup_imports(node_getter, node, lib_dir) { - const graph = new RollupImportGraph(node_getter, node); - const guard = new IllegalModuleGuard(lib_dir); - guard.assert_legal(graph); + return posixify(id); } /** - * Throw an error if a private module is imported from a client-side node. - * @param {import('vite').ModuleNode} node - * @param {string} lib_dir - * @param {Iterable} module_types File extensions to analyze in addition to the defaults: `.ts`, `.js`, etc. - * @returns {void} + * @param {string} str + * @param {number} times */ -export function prevent_illegal_vite_imports(node, lib_dir, module_types) { - const graph = new ViteImportGraph(get_module_types(module_types), node); - const guard = new IllegalModuleGuard(lib_dir); - guard.assert_legal(graph); +function repeat(str, times) { + return new Array(times + 1).join(str); } diff --git a/packages/kit/src/exports/vite/graph_analysis/index.spec.js b/packages/kit/src/exports/vite/graph_analysis/index.spec.js index 1b00de5a804a..2a3ee77382a5 100644 --- a/packages/kit/src/exports/vite/graph_analysis/index.spec.js +++ b/packages/kit/src/exports/vite/graph_analysis/index.spec.js @@ -1,191 +1,163 @@ -import { describe } from '../../../utils/unit_test.js'; +import { test } from 'uvu'; import * as assert from 'uvu/assert'; -import { IllegalModuleGuard } from './index.js'; -import path from 'path'; -import { normalizePath } from 'vite'; - -const CWD = process.cwd(); -const FAKE_LIB_DIR = normalizePath(path.join(CWD, 'lib')); -const DEV_VIRTUAL_DYNAMIC_ID = '/@id/__x00__$env/dynamic/private'; -const PROD_VIRTUAL_DYNAMIC_ID = '\0$env/dynamic/private'; -const DEV_VIRTUAL_STATIC_ID = '/@id/__x00__$env/static/private'; -const PROD_VIRTUAL_STATIC_ID = '\0$env/static/private'; -const USER_SERVER_ID = normalizePath(path.join(FAKE_LIB_DIR, 'test.server.js')); -const USER_SERVER_ID_NODE_MODULES = normalizePath(path.join(CWD, 'node_modules', 'test.server.js')); -const USER_SERVER_ID_OUTSIDE_ROOT = normalizePath(path.join(CWD, '..', 'test.server.js')); -const USER_SERVER_FOLDER_ID = normalizePath(path.join(FAKE_LIB_DIR, '/server/some/nested/path.js')); - -/** - * @template {any} T - * @param {Array} arr - * @returns {Generator} - */ -function* generator_from_array(arr) { - for (const item of arr) { - yield item; - } -} +import { module_guard } from './index.js'; /** - * @param {Array} nodes_to_insert - * @returns {import('./types').ImportGraph} + * + * @param {Record} graph + * @param {string} [expected_error] */ -function get_module_graph(...nodes_to_insert) { - return { - id: 'test.svelte', - dynamic: false, - children: generator_from_array([ - { - id: 'fine.js', - dynamic: false, - children: generator_from_array([ - { - id: 'also_fine.js', - dynamic: false, - children: generator_from_array([ - { - id: 'erstwhile.css', - dynamic: false, - children: generator_from_array([]) - }, - { - id: 'gruntled.js', - dynamic: false, - children: generator_from_array([]) - } - ]) - }, - { - id: 'somewhat_neat.js', - dynamic: false, - children: generator_from_array([]) - }, - { - id: 'blah.ts', - dynamic: false, - children: generator_from_array([]) - } - ]) - }, - { - id: 'something.svelte', - dynamic: false, - children: generator_from_array(nodes_to_insert) - }, - { - id: 'im_not_creative.hamburger', - dynamic: false, - children: generator_from_array([]) - } - ]) - }; -} - -describe('IllegalImportGuard', (test) => { - const guard = new IllegalModuleGuard(FAKE_LIB_DIR); - - test('assert succeeds for a graph with no illegal imports', () => { - assert.not.throws(() => guard.assert_legal(get_module_graph())); +function check(graph, expected_error) { + // @ts-expect-error + const context = /** @type {import('rollup').PluginContext} */ ({ + /** @param {string} id */ + getModuleInfo(id) { + return { + importedIds: [], + dynamicallyImportedIds: [], + ...graph[id] + }; + } }); - test('assert throws an error when importing $env/static/private in dev', () => { - const module_graph = get_module_graph({ - id: DEV_VIRTUAL_STATIC_ID, - dynamic: false, - children: generator_from_array([]) - }); - assert.throws( - () => guard.assert_legal(module_graph), - /.*Cannot import \$env\/static\/private into public-facing code:.*/gs - ); + const guard = module_guard(context, { + cwd: '~', + lib: '~/src/lib' }); - test('assert throws an error when importing $env/static/private in prod', () => { - const module_graph = get_module_graph({ - id: PROD_VIRTUAL_STATIC_ID, - dynamic: false, - children: generator_from_array([]) - }); - assert.throws( - () => guard.assert_legal(module_graph), - /.*Cannot import \$env\/static\/private into public-facing code:.*/gs - ); - }); - - test('assert throws an error when importing $env/dynamic/private in dev', () => { - const module_graph = get_module_graph({ - id: DEV_VIRTUAL_DYNAMIC_ID, - dynamic: false, - children: generator_from_array([]) - }); - assert.throws( - () => guard.assert_legal(module_graph), - /.*Cannot import \$env\/dynamic\/private into public-facing code:.*/gs - ); - }); + if (expected_error) { + try { + guard.check('~/src/entry'); + throw new Error('Expected an error'); + } catch (e) { + // @ts-expect-error + assert.equal(e.message, expected_error.replace(/^\t+/gm, '')); + } + } else { + guard.check('~/src/entry'); + } +} - test('assert throws an error when importing $env/dynamic/private in prod', () => { - const module_graph = get_module_graph({ - id: PROD_VIRTUAL_DYNAMIC_ID, - dynamic: false, - children: generator_from_array([]) - }); - assert.throws( - () => guard.assert_legal(module_graph), - /.*Cannot import \$env\/dynamic\/private into public-facing code:.*/gs - ); - }); +test('throws an error when importing $env/static/private', () => { + check( + { + '~/src/entry': { + importedIds: ['~/src/routes/+page.svelte'] + }, + '~/src/routes/+page.svelte': { + importedIds: ['\0$env/static/private'] + } + }, + `Cannot import \0$env/static/private into client-side code: + - src/routes/+page.svelte imports + - \0$env/static/private` + ); +}); - test('assert throws an error when importing a single server-only module', () => { - const module_graph = get_module_graph({ - id: USER_SERVER_ID, - dynamic: false, - children: generator_from_array([]) - }); +test('throws an error when dynamically importing $env/static/private', () => { + check( + { + '~/src/entry': { + importedIds: ['~/src/routes/+page.svelte'] + }, + '~/src/routes/+page.svelte': { + dynamicallyImportedIds: ['\0$env/static/private'] + } + }, + `Cannot import \0$env/static/private into client-side code: + - src/routes/+page.svelte dynamically imports + - \0$env/static/private` + ); +}); - assert.throws( - () => guard.assert_legal(module_graph), - /.*Cannot import \$lib\/test.server.js into public-facing code:.*/gs - ); - }); +test('throws an error when importing $env/dynamic/private', () => { + check( + { + '~/src/entry': { + importedIds: ['~/src/routes/+page.svelte'] + }, + '~/src/routes/+page.svelte': { + importedIds: ['\0$env/dynamic/private'] + } + }, + `Cannot import \0$env/dynamic/private into client-side code: + - src/routes/+page.svelte imports + - \0$env/dynamic/private` + ); +}); - test('assert throws an error when importing a module in the server-only folder', () => { - const module_graph = get_module_graph({ - id: USER_SERVER_FOLDER_ID, - dynamic: false, - children: generator_from_array([]) - }); +test('throws an error when dynamically importing $env/dynamic/private', () => { + check( + { + '~/src/entry': { + importedIds: ['~/src/routes/+page.svelte'] + }, + '~/src/routes/+page.svelte': { + dynamicallyImportedIds: ['\0$env/dynamic/private'] + } + }, + `Cannot import \0$env/dynamic/private into client-side code: + - src/routes/+page.svelte dynamically imports + - \0$env/dynamic/private` + ); +}); - assert.throws( - () => guard.assert_legal(module_graph), - /.*Cannot import \$lib\/server\/some\/nested\/path.js into public-facing code:.*/gs - ); - }); +test('throws an error when importing a .server.js module', () => { + check( + { + '~/src/entry': { + importedIds: ['~/src/routes/+page.svelte'] + }, + '~/src/routes/+page.svelte': { + importedIds: ['~/src/routes/illegal.server.js'] + }, + '~/src/routes/illegal.server.js': {} + }, + `Cannot import src/routes/illegal.server.js into client-side code: + - src/routes/+page.svelte imports + - src/routes/illegal.server.js` + ); +}); - test('assert ignores illegal server-only modules in node_modules', () => { - const module_graph = get_module_graph({ - id: USER_SERVER_ID_NODE_MODULES, - dynamic: false, - children: generator_from_array([]) - }); +test('throws an error when importing a $lib/server/**/*.js module', () => { + check( + { + '~/src/entry': { + importedIds: ['~/src/routes/+page.svelte'] + }, + '~/src/routes/+page.svelte': { + importedIds: ['~/src/lib/server/some/module.js'] + }, + '~/src/lib/server/some/module.js': {} + }, + `Cannot import $lib/server/some/module.js into client-side code: + - src/routes/+page.svelte imports + - $lib/server/some/module.js` + ); +}); - assert.not.throws(() => guard.assert_legal(module_graph)); +test('ignores .server.js files in node_modules', () => { + check({ + '~/src/entry': { + importedIds: ['~/src/routes/+page.svelte'] + }, + '~/src/routes/+page.svelte': { + importedIds: ['~/node_modules/illegal.server.js'] + }, + '~/node_modules/illegal.server.js': {} }); +}); - test('assert ignores illegal server-only modules outside the project root', () => { - const module_graph = get_module_graph({ - id: USER_SERVER_ID_OUTSIDE_ROOT, - dynamic: false, - children: generator_from_array([]) - }); - - assert.not.throws(() => guard.assert_legal(module_graph)); +test('ignores .server.js files outside the project root', () => { + check({ + '~/src/entry': { + importedIds: ['~/src/routes/+page.svelte'] + }, + '~/src/routes/+page.svelte': { + importedIds: ['/illegal.server.js'] + }, + '/illegal.server.js': {} }); }); -/* -We don't have a great way to mock Vite and Rollup's implementations of module graphs, so unit testing -ViteImportGraph and RollupImportGraph is kind of an exercise in "code coverage hubris" -- they're covered by -the integration tests, where Vite and Rollup can provide a useful graph implementation. If, in the future, we can find -a reason to unit test them, we can add those below. -*/ +test.run(); diff --git a/packages/kit/src/exports/vite/graph_analysis/utils.js b/packages/kit/src/exports/vite/graph_analysis/utils.js index f2caa3e04026..bdfa7a42b058 100644 --- a/packages/kit/src/exports/vite/graph_analysis/utils.js +++ b/packages/kit/src/exports/vite/graph_analysis/utils.js @@ -4,27 +4,3 @@ const query_pattern = /\?.*$/s; export function remove_query_from_id(path) { return path.replace(query_pattern, ''); } - -/** - * Vite does some weird things with import trees in dev - * for example, a Tailwind app.css will appear to import - * every file in the project. This isn't a problem for - * Rollup during build. - * @param {Iterable} config_module_types - */ -export const get_module_types = (config_module_types) => { - return new Set([ - '', - '.ts', - '.js', - '.svelte', - '.mts', - '.mjs', - '.cts', - '.cjs', - '.svelte.md', - '.svx', - '.md', - ...config_module_types - ]); -}; diff --git a/packages/kit/src/exports/vite/graph_analysis/utils.spec.js b/packages/kit/src/exports/vite/graph_analysis/utils.spec.js index 7aff1b9998a4..8b1feb7882a4 100644 --- a/packages/kit/src/exports/vite/graph_analysis/utils.spec.js +++ b/packages/kit/src/exports/vite/graph_analysis/utils.spec.js @@ -1,6 +1,6 @@ import { describe } from '../../../utils/unit_test.js'; import * as assert from 'uvu/assert'; -import { remove_query_from_id, get_module_types } from './utils.js'; +import { remove_query_from_id } from './utils.js'; describe('remove_query_string_from_path', (test) => { const module_ids = [ @@ -25,34 +25,3 @@ describe('remove_query_string_from_path', (test) => { }); }); }); - -describe('get_module_types', (test) => { - const base_expected_extensions = [ - '', - '.ts', - '.js', - '.svelte', - '.mts', - '.mjs', - '.cts', - '.cjs', - '.svelte.md', - '.svx', - '.md' - ]; - - test('returns correct base extensions', () => { - const module_types = get_module_types([]); - base_expected_extensions.forEach((extension) => { - assert.equal(module_types.has(extension), true); - }); - }); - - test('correctly extends base extensions', () => { - const additional_extensions = ['.foo', '.bar', '.baz']; - const module_types = get_module_types(additional_extensions); - base_expected_extensions.concat(additional_extensions).forEach((extension) => { - assert.equal(module_types.has(extension), true); - }); - }); -}); diff --git a/packages/kit/src/exports/vite/index.js b/packages/kit/src/exports/vite/index.js index e3d88ce8c592..951d8de681f3 100644 --- a/packages/kit/src/exports/vite/index.js +++ b/packages/kit/src/exports/vite/index.js @@ -15,9 +15,9 @@ import { runtime_directory, logger } from '../../core/utils.js'; import { find_deps, get_default_build_config } from './build/utils.js'; import { preview } from './preview/index.js'; import { get_config_aliases, get_app_aliases, get_env } from './utils.js'; -import { prevent_illegal_rollup_imports } from './graph_analysis/index.js'; import { fileURLToPath } from 'node:url'; import { create_static_module, create_dynamic_module } from '../../core/env.js'; +import { is_illegal, module_guard, normalize_id } from './graph_analysis/index.js'; const cwd = process.cwd(); @@ -292,7 +292,22 @@ function kit() { if (id.startsWith('$env/')) return `\0${id}`; }, - async load(id) { + async load(id, options) { + if (options?.ssr === false) { + const normalized_cwd = vite.normalizePath(cwd); + const normalized_lib = vite.normalizePath(svelte_config.kit.files.lib); + if ( + is_illegal(id, { + cwd: normalized_cwd, + node_modules: vite.normalizePath(path.join(cwd, 'node_modules')), + server: vite.normalizePath(path.join(normalized_lib, 'server')) + }) + ) { + const relative = normalize_id(id, normalized_lib, normalized_cwd); + throw new Error(`Cannot import ${relative} into client-side code`); + } + } + switch (id) { case '\0$env/static/private': return create_static_module('$env/static/private', env.private); @@ -350,20 +365,17 @@ function kit() { return; } + const guard = module_guard(this, { + cwd: vite.normalizePath(process.cwd()), + lib: vite.normalizePath(svelte_config.kit.files.lib) + }); + manifest_data.nodes.forEach((_node, i) => { const id = vite.normalizePath( path.resolve(svelte_config.kit.outDir, `generated/nodes/${i}.js`) ); - const module_node = this.getModuleInfo(id); - - if (module_node) { - prevent_illegal_rollup_imports( - this.getModuleInfo.bind(this), - module_node, - vite.normalizePath(svelte_config.kit.files.lib) - ); - } + guard.check(id); }); const verbose = vite_config.logLevel === 'info'; diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 0e7f5bb51cd1..73f0aed7f434 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -373,6 +373,8 @@ export function create_client({ target, base, trailing_slash }) { /** @param {import('./types').NavigationFinished} result */ function initialize(result) { + if (__SVELTEKIT_DEV__ && document.querySelector('vite-error-overlay')) return; + current = result.state; const style = document.querySelector('style[data-sveltekit]'); diff --git a/packages/kit/test/apps/basics/src/app.html b/packages/kit/test/apps/basics/src/app.html index bbc550f6a1b8..dd9910661cc3 100644 --- a/packages/kit/test/apps/basics/src/app.html +++ b/packages/kit/test/apps/basics/src/app.html @@ -5,9 +5,6 @@ - - %sveltekit.head% diff --git a/packages/kit/test/apps/dev-only/src/app.html b/packages/kit/test/apps/dev-only/src/app.html index bbc550f6a1b8..d169577acb70 100644 --- a/packages/kit/test/apps/dev-only/src/app.html +++ b/packages/kit/test/apps/dev-only/src/app.html @@ -5,12 +5,9 @@ - - %sveltekit.head% - - %sveltekit.body% + +
%sveltekit.body%
diff --git a/packages/kit/test/apps/dev-only/src/routes/env/dynamic-private-dynamic-import/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/illegal-imports/env/dynamic-private-dynamic-import/+page.svelte similarity index 100% rename from packages/kit/test/apps/dev-only/src/routes/env/dynamic-private-dynamic-import/+page.svelte rename to packages/kit/test/apps/dev-only/src/routes/illegal-imports/env/dynamic-private-dynamic-import/+page.svelte diff --git a/packages/kit/test/apps/dev-only/src/routes/env/dynamic-private/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/illegal-imports/env/dynamic-private/+page.svelte similarity index 100% rename from packages/kit/test/apps/dev-only/src/routes/env/dynamic-private/+page.svelte rename to packages/kit/test/apps/dev-only/src/routes/illegal-imports/env/dynamic-private/+page.svelte diff --git a/packages/kit/test/apps/dev-only/src/routes/env/static-private-dynamic-import/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/illegal-imports/env/static-private-dynamic-import/+page.svelte similarity index 100% rename from packages/kit/test/apps/dev-only/src/routes/env/static-private-dynamic-import/+page.svelte rename to packages/kit/test/apps/dev-only/src/routes/illegal-imports/env/static-private-dynamic-import/+page.svelte diff --git a/packages/kit/test/apps/dev-only/src/routes/env/static-private/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/illegal-imports/env/static-private/+page.svelte similarity index 100% rename from packages/kit/test/apps/dev-only/src/routes/env/static-private/+page.svelte rename to packages/kit/test/apps/dev-only/src/routes/illegal-imports/env/static-private/+page.svelte diff --git a/packages/kit/test/apps/dev-only/src/routes/server-only-folder/dynamic-import/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-folder/dynamic-import/+page.svelte similarity index 100% rename from packages/kit/test/apps/dev-only/src/routes/server-only-folder/dynamic-import/+page.svelte rename to packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-folder/dynamic-import/+page.svelte diff --git a/packages/kit/test/apps/dev-only/src/routes/server-only-folder/static-import/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-folder/static-import/+page.svelte similarity index 100% rename from packages/kit/test/apps/dev-only/src/routes/server-only-folder/static-import/+page.svelte rename to packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-folder/static-import/+page.svelte diff --git a/packages/kit/test/apps/dev-only/src/routes/server-only-modules/dynamic-import/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-modules/dynamic-import/+page.svelte similarity index 66% rename from packages/kit/test/apps/dev-only/src/routes/server-only-modules/dynamic-import/+page.svelte rename to packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-modules/dynamic-import/+page.svelte index ea20469dc4b2..5e763368e26f 100644 --- a/packages/kit/test/apps/dev-only/src/routes/server-only-modules/dynamic-import/+page.svelte +++ b/packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-modules/dynamic-import/+page.svelte @@ -1,5 +1,5 @@ {#await mod then resolved} diff --git a/packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-modules/illegal.server.js b/packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-modules/illegal.server.js new file mode 100644 index 000000000000..770268db581d --- /dev/null +++ b/packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-modules/illegal.server.js @@ -0,0 +1 @@ +export const should_explode = 'boom'; diff --git a/packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-modules/static-import/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-modules/static-import/+page.svelte new file mode 100644 index 000000000000..43bb66e92f6d --- /dev/null +++ b/packages/kit/test/apps/dev-only/src/routes/illegal-imports/server-only-modules/static-import/+page.svelte @@ -0,0 +1,5 @@ + + +

{should_explode}

diff --git a/packages/kit/test/apps/dev-only/src/routes/server-only-modules/static-import/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/server-only-modules/static-import/+page.svelte deleted file mode 100644 index e0e7bff39ae1..000000000000 --- a/packages/kit/test/apps/dev-only/src/routes/server-only-modules/static-import/+page.svelte +++ /dev/null @@ -1,5 +0,0 @@ - - -

{should_explode}

diff --git a/packages/kit/test/apps/dev-only/svelte.config.js b/packages/kit/test/apps/dev-only/svelte.config.js index bded48544036..821c14379ec8 100644 --- a/packages/kit/test/apps/dev-only/svelte.config.js +++ b/packages/kit/test/apps/dev-only/svelte.config.js @@ -1,4 +1,6 @@ /** @type {import('@sveltejs/kit').Config} */ -const config = {}; +const config = { + kit: {} +}; export default config; diff --git a/packages/kit/test/apps/dev-only/test/test.js b/packages/kit/test/apps/dev-only/test/test.js index 7010d7a5149f..cc8f615f7e2c 100644 --- a/packages/kit/test/apps/dev-only/test/test.js +++ b/packages/kit/test/apps/dev-only/test/test.js @@ -3,66 +3,78 @@ import { test } from '../../../utils.js'; /** @typedef {import('@playwright/test').Response} Response */ -test.describe.configure({ mode: 'parallel' }); +test.describe.serial('Illegal imports', () => { + test.skip(({ javaScriptEnabled }) => !process.env.DEV || !javaScriptEnabled); -test.describe('$env', () => { - test('$env/dynamic/private is not statically importable from the client', async ({ request }) => { - const resp = await request.get('/env/dynamic-private'); - expect(await resp.text()).toMatch( - /.*Cannot import \$env\/dynamic\/private into public-facing code:.*/gs + test('$env/dynamic/private is not statically importable from the client', async ({ page }) => { + await page.goto('/illegal-imports/env/dynamic-private', { + wait_for_started: false + }); + expect(await page.textContent('.message-body')).toBe( + 'Cannot import \0$env/dynamic/private into client-side code' ); }); - test('$env/dynamic/private is not dynamically importable from the client', async ({ - request - }) => { - const resp = await request.get('/env/dynamic-private-dynamic-import'); - expect(await resp.text()).toMatch( - /.*Cannot import \$env\/dynamic\/private into public-facing code:.*/gs + test('$env/dynamic/private is not dynamically importable from the client', async ({ page }) => { + await page.goto('/illegal-imports/env/dynamic-private-dynamic-import', { + wait_for_started: false + }); + expect(await page.textContent('.message-body')).toBe( + 'Cannot import \0$env/dynamic/private into client-side code' ); }); - test('$env/static/private is not statically importable from the client', async ({ request }) => { - const resp = await request.get('/env/static-private'); - expect(await resp.text()).toMatch( - /.*Cannot import \$env\/static\/private into public-facing code:.*/gs + test('$env/static/private is not statically importable from the client', async ({ page }) => { + await page.goto('/illegal-imports/env/static-private', { + wait_for_started: false + }); + expect(await page.textContent('.message-body')).toBe( + 'Cannot import \0$env/static/private into client-side code' ); }); - test('$env/static/private is not dynamically importable from the client', async ({ request }) => { - const resp = await request.get('/env/static-private-dynamic-import'); - expect(await resp.text()).toMatch( - /.*Cannot import \$env\/static\/private into public-facing code:.*/gs + test('$env/static/private is not dynamically importable from the client', async ({ page }) => { + await page.goto('/illegal-imports/env/static-private-dynamic-import', { + wait_for_started: false + }); + expect(await page.textContent('.message-body')).toBe( + 'Cannot import \0$env/static/private into client-side code' ); }); -}); -test.describe('server-only modules', () => { - test('server-only module is not statically importable from the client', async ({ request }) => { - const resp = await request.get('/server-only-modules/static-import'); - expect(await resp.text()).toMatch( - /.*Cannot import \$lib\/test.server.js into public-facing code:.*/gs + test('server-only module is not statically importable from the client', async ({ page }) => { + await page.goto('/illegal-imports/server-only-modules/static-import', { + wait_for_started: false + }); + expect(await page.textContent('.message-body')).toBe( + 'Cannot import src/routes/illegal-imports/server-only-modules/illegal.server.js into client-side code' ); }); - test('server-only module is not dynamically importable from the client', async ({ request }) => { - const resp = await request.get('/server-only-modules/dynamic-import'); - expect(await resp.text()).toMatch( - /.*Cannot import \$lib\/test.server.js into public-facing code:.*/gs + + test('server-only module is not dynamically importable from the client', async ({ page }) => { + await page.goto('/illegal-imports/server-only-modules/dynamic-import', { + wait_for_started: false + }); + expect(await page.textContent('.message-body')).toBe( + 'Cannot import src/routes/illegal-imports/server-only-modules/illegal.server.js into client-side code' ); }); -}); -test.describe('server-only folder', () => { - test('server-only folder is not statically importable from the client', async ({ request }) => { - const resp = await request.get('/server-only-folder/static-import'); - expect(await resp.text()).toMatch( - /.*Cannot import \$lib\/server\/blah\/test.js into public-facing code:.*/gs + test('server-only folder is not statically importable from the client', async ({ page }) => { + await page.goto('/illegal-imports/server-only-folder/static-import', { + wait_for_started: false + }); + expect(await page.textContent('.message-body')).toBe( + 'Cannot import $lib/server/blah/test.js into client-side code' ); }); - test('server-only folder is not dynamically importable from the client', async ({ request }) => { - const resp = await request.get('/server-only-folder/dynamic-import'); - expect(await resp.text()).toMatch( - /.*Cannot import \$lib\/server\/blah\/test.js into public-facing code:.*/gs + + test('server-only folder is not dynamically importable from the client', async ({ page }) => { + await page.goto('/illegal-imports/server-only-folder/dynamic-import', { + wait_for_started: false + }); + expect(await page.textContent('.message-body')).toBe( + 'Cannot import $lib/server/blah/test.js into client-side code' ); }); }); diff --git a/packages/kit/test/build-errors/env.spec.js b/packages/kit/test/build-errors/env.spec.js index 1dba0a958ec4..c0fec16a4ecd 100644 --- a/packages/kit/test/build-errors/env.spec.js +++ b/packages/kit/test/build-errors/env.spec.js @@ -11,7 +11,7 @@ test('$env/dynamic/private is not statically importable from the client', () => stdio: 'pipe', timeout: 15000 }), - /.*Cannot import \$env\/dynamic\/private into public-facing code:.*/gs + /.*Cannot import \0\$env\/dynamic\/private into client-side code:.*/gs ); }); @@ -23,7 +23,7 @@ test('$env/dynamic/private is not dynamically importable from the client', () => stdio: 'pipe', timeout: 15000 }), - /.*Cannot import \$env\/dynamic\/private into public-facing code:.*/gs + /.*Cannot import \0\$env\/dynamic\/private into client-side code:.*/gs ); }); @@ -35,7 +35,7 @@ test('$env/static/private is not statically importable from the client', () => { stdio: 'pipe', timeout: 15000 }), - /.*Cannot import \$env\/static\/private into public-facing code:.*/gs + /.*Cannot import \0\$env\/static\/private into client-side code:.*/gs ); }); @@ -47,7 +47,7 @@ test('$env/static/private is not dynamically importable from the client', () => stdio: 'pipe', timeout: 15000 }), - /.*Cannot import \$env\/static\/private into public-facing code:.*/gs + /.*Cannot import \0\$env\/static\/private into client-side code:.*/gs ); }); diff --git a/packages/kit/test/build-errors/server-only.spec.js b/packages/kit/test/build-errors/server-only.spec.js index f11b33beb36a..7e620820eb5b 100644 --- a/packages/kit/test/build-errors/server-only.spec.js +++ b/packages/kit/test/build-errors/server-only.spec.js @@ -11,7 +11,7 @@ test('$lib/*.server.* is not statically importable from the client', () => { stdio: 'pipe', timeout: 15000 }), - /.*Cannot import \$lib\/test.server.js into public-facing code:.*/gs + /.*Cannot import \$lib\/test.server.js into client-side code:.*/gs ); }); @@ -23,7 +23,7 @@ test('$lib/*.server.* is not dynamically importable from the client', () => { stdio: 'pipe', timeout: 15000 }), - /.*Cannot import \$lib\/test.server.js into public-facing code:.*/gs + /.*Cannot import \$lib\/test.server.js into client-side code:.*/gs ); }); @@ -35,7 +35,7 @@ test('$lib/server/* is not statically importable from the client', () => { stdio: 'pipe', timeout: 15000 }), - /.*Cannot import \$lib\/server\/something\/test.js into public-facing code:.*/gs + /.*Cannot import \$lib\/server\/something\/test.js into client-side code:.*/gs ); }); @@ -47,7 +47,7 @@ test('$lib/server/* is not dynamically importable from the client', () => { stdio: 'pipe', timeout: 15000 }), - /.*Cannot import \$lib\/server\/something\/test.js into public-facing code:.*/gs + /.*Cannot import \$lib\/server\/something\/test.js into client-side code:.*/gs ); }); diff --git a/packages/kit/test/utils.d.ts b/packages/kit/test/utils.d.ts index a1958487e261..31d83058f9e4 100644 --- a/packages/kit/test/utils.d.ts +++ b/packages/kit/test/utils.d.ts @@ -26,6 +26,12 @@ export const test: TestType< * `handleError` defines the shape */ read_errors(href: string): Record; + page: PlaywrightTestArgs['page'] & { + goto: ( + url: string, + opts?: Parameters[1] & { wait_for_started?: boolean } + ) => ReturnType; + }; }, PlaywrightWorkerArgs & PlaywrightWorkerOptions >; diff --git a/packages/kit/test/utils.js b/packages/kit/test/utils.js index a60ec8e56e5d..4bfc9ebdc94e 100644 --- a/packages/kit/test/utils.js +++ b/packages/kit/test/utils.js @@ -90,7 +90,7 @@ export const test = base.extend({ // @ts-expect-error page[fn] = async function (...args) { const res = await page_fn.call(page, ...args); - if (javaScriptEnabled) { + if (javaScriptEnabled && args[1]?.wait_for_started !== false) { await page.waitForSelector('body.started', { timeout: 5000 }); } return res;