From 034b4157f5da8a309883991d6a3f45c8519624c4 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Sun, 11 Sep 2022 00:02:30 +0200 Subject: [PATCH 1/6] chore: Move uvu describe into utils file --- packages/kit/src/utils/unit_test.js | 12 ++++++++++++ packages/kit/src/utils/url.spec.js | 13 +------------ 2 files changed, 13 insertions(+), 12 deletions(-) create mode 100644 packages/kit/src/utils/unit_test.js diff --git a/packages/kit/src/utils/unit_test.js b/packages/kit/src/utils/unit_test.js new file mode 100644 index 000000000000..af13bec239da --- /dev/null +++ b/packages/kit/src/utils/unit_test.js @@ -0,0 +1,12 @@ +import { suite } from 'uvu'; + +/** + * + * @param {string} name + * @param {(suite: import('uvu').Test) => void} fn + */ +export function describe(name, fn) { + const s = suite(name); + fn(s); + s.run(); +} \ No newline at end of file diff --git a/packages/kit/src/utils/url.spec.js b/packages/kit/src/utils/url.spec.js index 3d960305536c..4c359b224bad 100644 --- a/packages/kit/src/utils/url.spec.js +++ b/packages/kit/src/utils/url.spec.js @@ -1,18 +1,7 @@ -import { suite } from 'uvu'; import * as assert from 'uvu/assert'; +import { describe } from './unit_test.js'; import { resolve, normalize_path, make_trackable, disable_search } from './url.js'; -/** - * - * @param {string} name - * @param {(suite: import('uvu').Test) => void} fn - */ -function describe(name, fn) { - const s = suite(name); - fn(s); - s.run(); -} - describe('resolve', (test) => { test('resolves a root-relative path', () => { assert.equal(resolve('/a/b/c', '/x/y/z'), '/x/y/z'); From 7f0871d26501b2cbd3167ce018e2df43a9c36a11 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Sun, 11 Sep 2022 01:33:06 +0200 Subject: [PATCH 2/6] feat: Refactored module graph analysis for unit testing --- packages/kit/src/exports/vite/dev/index.js | 2 +- .../src/exports/vite/graph_analysis/index.js | 277 +++++++++++++++++ .../exports/vite/graph_analysis/index.spec.js | 184 ++++++++++++ .../exports/vite/graph_analysis/types.d.ts | 5 + .../src/exports/vite/graph_analysis/utils.js | 30 ++ .../exports/vite/graph_analysis/utils.spec.js | 58 ++++ packages/kit/src/exports/vite/index.js | 3 +- packages/kit/src/exports/vite/utils.js | 283 +----------------- packages/kit/src/exports/vite/utils.spec.js | 170 +---------- packages/kit/types/internal.d.ts | 6 - 10 files changed, 559 insertions(+), 459 deletions(-) create mode 100644 packages/kit/src/exports/vite/graph_analysis/index.js create mode 100644 packages/kit/src/exports/vite/graph_analysis/index.spec.js create mode 100644 packages/kit/src/exports/vite/graph_analysis/types.d.ts create mode 100644 packages/kit/src/exports/vite/graph_analysis/utils.js create mode 100644 packages/kit/src/exports/vite/graph_analysis/utils.spec.js diff --git a/packages/kit/src/exports/vite/dev/index.js b/packages/kit/src/exports/vite/dev/index.js index 8bfd4eaf3e4f..3c28acb480d8 100644 --- a/packages/kit/src/exports/vite/dev/index.js +++ b/packages/kit/src/exports/vite/dev/index.js @@ -11,7 +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 '../utils.js'; +import { prevent_illegal_vite_imports } from '../graph_analysis/index.js'; import { compact } from '../../../utils/array.js'; import { normalizePath } from 'vite'; diff --git a/packages/kit/src/exports/vite/graph_analysis/index.js b/packages/kit/src/exports/vite/graph_analysis/index.js new file mode 100644 index 000000000000..7a380125e4c5 --- /dev/null +++ b/packages/kit/src/exports/vite/graph_analysis/index.js @@ -0,0 +1,277 @@ +import path from 'path'; +import { normalizePath } from 'vite'; +import { remove_query_from_id, get_module_types } from './utils.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_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')); + } + + /** + * 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(); + } + + /** + * `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; + } + + /** + * `true` if the provided ID represents a Kit-defined server-only module, else `false`. + * @param {string} module_id + * @returns {boolean} + */ + #is_kit_illegal(module_id) { + return ILLEGAL_IMPORTS.has(module_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; + + // files outside the project root are ignored + if (!module_id.startsWith(CWD_ID)) return false; + + // so are files inside node_modules + if (module_id.startsWith(NODE_MODULES_ID)) return false; + + return ILLEGAL_MODULE_NAME_PATTERN.test(path.basename(module_id)); + } + + /** + * @param {string} str + * @param {number} times + */ + #repeat(str, times) { + return new Array(times + 1).join(str); + } + + /** + * 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'; + + 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') }; + } + + 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 client-side 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(); + } + + *#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); + } + } +} + +/** @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; + } + + get children() { + return this.#children(); + } + + *#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); +} + +/** + * 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} + */ +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); +} diff --git a/packages/kit/src/exports/vite/graph_analysis/index.spec.js b/packages/kit/src/exports/vite/graph_analysis/index.spec.js new file mode 100644 index 000000000000..3f2d0527bae2 --- /dev/null +++ b/packages/kit/src/exports/vite/graph_analysis/index.spec.js @@ -0,0 +1,184 @@ +import { describe } from '../../../utils/unit_test.js'; +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; + } +} + +/** + * @param {Array} nodes_to_insert + * @returns {import('./types').ImportGraph} + */ +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())); + }); + + 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 client-side code:.*/gs + ); + }); + + 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 client-side 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 client-side code:.*/gs + ); + }); + + 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 client-side code:.*/gs + ); + }); + + 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([]) + }); + + assert.throws( + () => guard.assert_legal(module_graph), + /.*Cannot import \$lib\/test.server.js into client-side code:.*/gs + ); + }); + + 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([]) + }); + + assert.throws( + () => guard.assert_legal(module_graph), + /.*Cannot import \$lib\/server\/some\/nested\/path.js into client-side code:.*/gs + ); + }); + + 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([]) + }); + + assert.not.throws(() => guard.assert_legal(module_graph)); + }); + + 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)); + }); +}); diff --git a/packages/kit/src/exports/vite/graph_analysis/types.d.ts b/packages/kit/src/exports/vite/graph_analysis/types.d.ts new file mode 100644 index 000000000000..1239fec437e1 --- /dev/null +++ b/packages/kit/src/exports/vite/graph_analysis/types.d.ts @@ -0,0 +1,5 @@ +export interface ImportGraph { + readonly id: string; + readonly dynamic: boolean; + readonly children: Generator; +} diff --git a/packages/kit/src/exports/vite/graph_analysis/utils.js b/packages/kit/src/exports/vite/graph_analysis/utils.js new file mode 100644 index 000000000000..f2caa3e04026 --- /dev/null +++ b/packages/kit/src/exports/vite/graph_analysis/utils.js @@ -0,0 +1,30 @@ +const query_pattern = /\?.*$/s; + +/** @param {string} path */ +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 new file mode 100644 index 000000000000..7aff1b9998a4 --- /dev/null +++ b/packages/kit/src/exports/vite/graph_analysis/utils.spec.js @@ -0,0 +1,58 @@ +import { describe } from '../../../utils/unit_test.js'; +import * as assert from 'uvu/assert'; +import { remove_query_from_id, get_module_types } from './utils.js'; + +describe('remove_query_string_from_path', (test) => { + const module_ids = [ + '$env/static/private', + 'some-normal-js-module.js', + 'c:\\\\some\\stupid\\windows\\path.js', + '/some/normal/linux/path.js' + ]; + const query_module_ids = module_ids.map((module_id) => `${module_id}?hello=world,something=else`); + + test('does nothing to valid IDs', () => { + module_ids.forEach((id) => { + const query_stringless = remove_query_from_id(id); + assert.equal(query_stringless, id); + }); + }); + + test('removes querystring from paths with querystrings at the end', () => { + query_module_ids.forEach((id, i) => { + const query_stringless = remove_query_from_id(id); + assert.equal(query_stringless, module_ids[i]); + }); + }); +}); + +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 b7eda46b4402..c6b99a81527d 100644 --- a/packages/kit/src/exports/vite/index.js +++ b/packages/kit/src/exports/vite/index.js @@ -14,7 +14,8 @@ import { generate_manifest } from '../../core/generate_manifest/index.js'; 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_aliases, prevent_illegal_rollup_imports, get_env } from './utils.js'; +import { get_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'; diff --git a/packages/kit/src/exports/vite/utils.js b/packages/kit/src/exports/vite/utils.js index fc4ff215935b..cfbafa7b616f 100644 --- a/packages/kit/src/exports/vite/utils.js +++ b/packages/kit/src/exports/vite/utils.js @@ -1,232 +1,8 @@ import path from 'path'; -import { loadConfigFromFile, loadEnv, normalizePath } from 'vite'; +import { loadConfigFromFile, loadEnv } from 'vite'; import { runtime_directory } from '../../core/utils.js'; import { posixify } from '../../utils/filesystem.js'; -class IllegalModuleGuard { - /** @type {string} */ - #lib_dir; - - /** @type {string} */ - #server_dir; - - /** @type {string} */ - #node_modules_dir = normalizePath(path.resolve(process.cwd(), 'node_modules')); - - /** @type {string} */ - #cwd = normalizePath(process.cwd()); - - /** @type {Set} */ - #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 - ]); - - /** @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')); - } - - /** - * @param {import('types').ImportNode} node - * @returns {void} - */ - assert_legal(node) { - this.#chain.push(node); - for (const child of node.children) { - if (this.#is_illegal(child.name)) { - this.#chain.push(child); - throw new Error(this.#format_illegal_import_chain(this.#chain)); - } - this.assert_legal(child); - } - this.#chain.pop(); - } - - /** - * `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; - } - - /** - * `true` if the provided ID represents a Kit-defined server-only module, else `false`. - * @param {string} module_id - * @returns {boolean} - */ - #is_kit_illegal(module_id) { - return this.#illegal_imports.has(module_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; - - // files outside the project root are ignored - if (!module_id.startsWith(this.#cwd)) return false; - - // so are files inside node_modules - if (module_id.startsWith(this.#node_modules_dir)) return false; - - return /.*\.server\..+/.test(path.basename(module_id)); - } - - /** - * @param {string} str - * @param {number} times - */ - #repeat(str, times) { - return new Array(times + 1).join(str); - } - - /** - * Create a formatted error for an illegal import. - * @param {Array<{name: string, dynamic: boolean}>} stack - */ - #format_illegal_import_chain(stack) { - const dev_virtual_prefix = '/@id/__x00__'; - const prod_virtual_prefix = '\0'; - - stack = stack.map((file) => { - if (file.name.startsWith(dev_virtual_prefix)) { - return { ...file, name: file.name.replace(dev_virtual_prefix, '') }; - } - if (file.name.startsWith(prod_virtual_prefix)) { - return { ...file, name: file.name.replace(prod_virtual_prefix, '') }; - } - if (file.name.startsWith(this.#lib_dir)) { - return { ...file, name: file.name.replace(this.#lib_dir, '$lib') }; - } - - return { ...file, name: path.relative(process.cwd(), file.name) }; - }); - - const pyramid = stack - .map( - (file, i) => - `${this.#repeat(' ', i * 2)}- ${file.name} ${ - file.dynamic ? '(imported by parent dynamically)' : '' - }` - ) - .join('\n'); - - return `Cannot import ${stack.at(-1)?.name} into client-side code:\n${pyramid}`; - } -} - -class RollupImportGraph { - /** @type {(id: string) => import('rollup').ModuleInfo | null} */ - #node_getter; - - /** @type {import('rollup').ModuleInfo} */ - #module_info; - - /** @type {string} */ - name; - - /** @type {boolean} */ - dynamic; - - /** @type {Set} */ - #seen; - - /** - * @param {(id: string) => import('rollup').ModuleInfo | null} node_getter - * @param {import('rollup').ModuleInfo} node - * @param {boolean} dynamic - * @param {Set} seen - */ - constructor(node_getter, node, dynamic = false, seen = new Set()) { - this.#node_getter = node_getter; - this.#module_info = node; - this.name = remove_query_from_path(normalizePath(node.id)); - this.dynamic = dynamic; - this.#seen = seen; - void (/** @type {import('types').ImportNode} */ (this)); - } - - get children() { - return this.#children(); - } - - *#children() { - if (this.#seen.has(this.name)) return; - this.#seen.add(this.name); - for (const id of this.#module_info.importedIds) { - const child = this.#node_getter(id); - if (child === null) return; - yield new RollupImportGraph(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 new RollupImportGraph(this.#node_getter, child, true, this.#seen); - } - } -} - -class ViteImportGraph { - /** @type {Set} */ - #module_types; - - /** @type {import('vite').ModuleNode} */ - #module_info; - - /** @type {string} */ - name; - - /** @type {Set} */ - #seen; - - /** - * @param {Set} module_types Module types to analyze, eg '.js', '.ts', etc. - * @param {import('vite').ModuleNode} node - * @param {Set} seen - */ - constructor(module_types, node, seen = new Set()) { - this.#module_types = module_types; - this.#module_info = node; - this.name = remove_query_from_path(normalizePath(node.id ?? '')); - this.#seen = seen; - void (/** @type {import('types').ImportNode} */ (this)); - } - - get dynamic() { - return false; - } - - get children() { - return this.#children(); - } - - *#children() { - if (this.#seen.has(this.name)) return; - this.#seen.add(this.name); - for (const child of this.#module_info.importedModules) { - if (!this.#module_types.has(path.extname(this.name))) { - continue; - } - yield new ViteImportGraph(this.#module_types, child, this.#seen); - } - } -} - /** * @param {import('vite').ResolvedConfig} config * @param {import('vite').ConfigEnv} config_env @@ -379,60 +155,3 @@ export function get_env(env_config, mode) { private: Object.fromEntries(entries.filter(([k]) => !k.startsWith(env_config.publicPrefix))) }; } - -const query_pattern = /\?.*$/s; - -/** @param {string} path */ -function remove_query_from_path(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 - */ -const get_module_types = (config_module_types) => { - return new Set([ - '', - '.ts', - '.js', - '.svelte', - '.mts', - '.mjs', - '.cts', - '.cjs', - '.svelte.md', - '.svx', - '.md', - ...config_module_types - ]); -}; - -/** - * 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); -} - -/** - * 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} - */ -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); -} diff --git a/packages/kit/src/exports/vite/utils.spec.js b/packages/kit/src/exports/vite/utils.spec.js index 558d5d563956..f2160386d15d 100644 --- a/packages/kit/src/exports/vite/utils.spec.js +++ b/packages/kit/src/exports/vite/utils.spec.js @@ -3,15 +3,7 @@ import { test } from 'uvu'; import * as assert from 'uvu/assert'; import { validate_config } from '../../core/config/index.js'; import { posixify } from '../../utils/filesystem.js'; -import { - deep_merge, - get_aliases, - merge_vite_configs, - prevent_illegal_rollup_imports, - prevent_illegal_vite_imports -} from './utils.js'; - -const illegal_id = '/@id/__x00__$env/dynamic/private'; +import { deep_merge, get_aliases, merge_vite_configs } from './utils.js'; test('basic test no conflicts', async () => { const merged = deep_merge( @@ -240,164 +232,4 @@ test('transform kit.alias to resolve.alias', () => { ]); }); -/** @typedef {{id: string, importedIds: Array, dynamicallyImportedIds: Array }} RollupNode */ - -/** @type {(id: string) => RollupNode | null} */ -const rollup_node_getter = (id) => { - /** @type {{[key: string]: RollupNode}} */ - const nodes = { - '/test/path1.js': { - id: '/test/path1.js', - importedIds: ['/test/path2.js', '/test/path3.js'], - dynamicallyImportedIds: ['/test/path4.js', '/test/path5.js'] - }, - '/test/path2.js': { - id: '/test/path2.js', - importedIds: ['/test/path3.js'], - dynamicallyImportedIds: ['/test/path5.js'] - }, - '/test/path3.js': { - id: '/test/path3.js', - importedIds: ['/test/path5.js'], - dynamicallyImportedIds: ['/test/path1.js'] - }, - '/test/path4.js': { - id: '/test/path4.js', - importedIds: ['/test/path5.js'], - dynamicallyImportedIds: ['/test/path3.js'] - }, - '/test/path5.js': { - id: '/test/path5.js', - importedIds: ['/test/path1.js'], - dynamicallyImportedIds: ['/test/path3.js'] - }, - '/bad/static.js': { - id: '/bad/static.js', - importedIds: ['/statically-imports/bad/module.js'], - dynamicallyImportedIds: ['/test/path1.js'] - }, - '/statically-imports/bad/module.js': { - id: '/statically-imports/bad/module.js', - importedIds: [illegal_id], - dynamicallyImportedIds: ['/test/path2.js'] - }, - '/bad/dynamic.js': { - id: '/bad/dynamic.js', - importedIds: ['/dynamically-imports/bad/module.js'], - dynamicallyImportedIds: ['/test/path1.js'] - }, - '/dynamically-imports/bad/module.js': { - id: '/dynamically-imports/bad/module.js', - importedIds: ['/test/path5.js'], - dynamicallyImportedIds: ['/test/path2.js', illegal_id] - }, - [illegal_id]: { - id: illegal_id, - importedIds: [], - dynamicallyImportedIds: [] - } - }; - return nodes[id] ?? null; -}; - -const ok_rollup_node = rollup_node_getter('/test/path1.js'); -const bad_rollup_node_static = rollup_node_getter('/bad/static.js'); -const bad_rollup_node_dynamic = rollup_node_getter('/bad/dynamic.js'); - -test('allows ok rollup imports', () => { - assert.not.throws(() => { - prevent_illegal_rollup_imports( - // @ts-expect-error - rollup_node_getter, - ok_rollup_node, - 'should_not_match_anything' - ); - }); -}); - -test('does not allow bad static rollup imports', () => { - assert.throws(() => { - prevent_illegal_rollup_imports( - // @ts-expect-error - rollup_node_getter, - bad_rollup_node_static, - 'should_not_match_anything' - ); - }); -}); - -test('does not allow bad dynamic rollup imports', () => { - assert.throws(() => { - prevent_illegal_rollup_imports( - // @ts-expect-error - rollup_node_getter, - bad_rollup_node_dynamic, - 'should_not_match_anything' - ); - }); -}); - -/** @typedef {{id: string, importedModules: Set}} ViteNode */ - -/** @type {ViteNode} */ -const ok_vite_node = { - id: '/test/ok.js', - importedModules: new Set([ - { - id: '/test/path1.js', - importedModules: new Set([ - { - id: '/test/path2.js', - importedModules: new Set() - } - ]) - }, - { id: '/test/path3.js', importedModules: new Set() } - ]) -}; - -/** @type {ViteNode} */ -const bad_vite_node = { - id: '/test/bad-static.js', - importedModules: new Set([ - { - id: '/test/path1.js', - importedModules: new Set([ - { - id: '/test/path2.js', - importedModules: new Set([ - { - id: illegal_id, - importedModules: new Set() - } - ]) - } - ]) - }, - { id: '/test/path3.js', importedModules: new Set() } - ]) -}; - -test('allows ok vite imports', () => { - assert.not.throws(() => { - prevent_illegal_vite_imports( - // @ts-expect-error - ok_vite_node, - 'should_not_match_anything', - [] - ); - }); -}); - -test('does not allow bad static rollup imports', () => { - assert.throws(() => { - prevent_illegal_vite_imports( - // @ts-expect-error - bad_vite_node, - 'should_not_match_anything', - [] - ); - }); -}); - test.run(); diff --git a/packages/kit/types/internal.d.ts b/packages/kit/types/internal.d.ts index 0e72dda4b93c..aebf0be71669 100644 --- a/packages/kit/types/internal.d.ts +++ b/packages/kit/types/internal.d.ts @@ -100,12 +100,6 @@ export interface ClientHooks { handleError: HandleClientError; } -export interface ImportNode { - readonly name: string; - readonly dynamic: boolean; - readonly children: Generator; -} - export class InternalServer extends Server { init(options: ServerInitOptions): Promise; respond( From 39ff07db338cba74e49d97d38d2cfaa8184c939a Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Sun, 11 Sep 2022 01:36:45 +0200 Subject: [PATCH 3/6] chore: Comment explaining lack of codecov --- packages/kit/src/exports/vite/graph_analysis/index.spec.js | 7 +++++++ 1 file changed, 7 insertions(+) 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 3f2d0527bae2..3e2b98c1f8e0 100644 --- a/packages/kit/src/exports/vite/graph_analysis/index.spec.js +++ b/packages/kit/src/exports/vite/graph_analysis/index.spec.js @@ -182,3 +182,10 @@ describe('IllegalImportGuard', (test) => { assert.not.throws(() => guard.assert_legal(module_graph)); }); }); + +/* +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. +*/ From edf07c9b13a3931a1c6c74b3df23c08d503f20ac Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Sun, 11 Sep 2022 01:37:14 +0200 Subject: [PATCH 4/6] fix: style --- packages/kit/src/utils/unit_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/kit/src/utils/unit_test.js b/packages/kit/src/utils/unit_test.js index af13bec239da..efc6ee8289c7 100644 --- a/packages/kit/src/utils/unit_test.js +++ b/packages/kit/src/utils/unit_test.js @@ -9,4 +9,4 @@ export function describe(name, fn) { const s = suite(name); fn(s); s.run(); -} \ No newline at end of file +} From 4875d905c666090ff8f39cbac73923d60e65e4f3 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Sun, 11 Sep 2022 01:39:17 +0200 Subject: [PATCH 5/6] changeset --- .changeset/lovely-sloths-flow.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/lovely-sloths-flow.md diff --git a/.changeset/lovely-sloths-flow.md b/.changeset/lovely-sloths-flow.md new file mode 100644 index 000000000000..c2cfa9d81e31 --- /dev/null +++ b/.changeset/lovely-sloths-flow.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +[chore] Refactor graph analysis for better unit tests From 6a7e30404ac31429b4984699bc2d9b8396f5dbfb Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Sun, 11 Sep 2022 08:01:54 +0200 Subject: [PATCH 6/6] Update packages/kit/src/utils/unit_test.js Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com> --- packages/kit/src/utils/unit_test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/kit/src/utils/unit_test.js b/packages/kit/src/utils/unit_test.js index efc6ee8289c7..eca30f1b4f35 100644 --- a/packages/kit/src/utils/unit_test.js +++ b/packages/kit/src/utils/unit_test.js @@ -1,7 +1,6 @@ import { suite } from 'uvu'; /** - * * @param {string} name * @param {(suite: import('uvu').Test) => void} fn */