diff --git a/server/scripts/checkImports.js b/server/scripts/checkImports.js index 130e29a..958c67c 100644 --- a/server/scripts/checkImports.js +++ b/server/scripts/checkImports.js @@ -28,19 +28,23 @@ // because a test that reaches into core's tree is a test that passes on this // machine and nowhere else. // -// Run over the SERVER half. The client half's equivalent is its Vite build: -// the four shared dependencies are `external`, and anything else that stays a -// bare import in the emitted chunk is unresolvable in the browser. +// Run over the SERVER half. The client half's equivalents are its Vite build, +// which fails if a shared dependency resolves into node_modules, and +// client/scripts/checkExternals.js, which asks the built chunk whether any bare +// specifier survived. const fs = require('fs') const path = require('path') -const { builtinModules } = require('module') +// Node's own answer, not a list reconstructed from `builtinModules`. That list +// omits `test` on Node 20 and includes it on Node 24, so a suite that requires +// `node:test` passed locally and failed in CI on the very first run — reported +// as the module boundary being broken, which it was not. `isBuiltin` is the +// authoritative check and handles the `node:` prefix itself. +const { isBuiltin } = require('module') const MODULE_ROOT = path.resolve(__dirname, '..', '..') const SERVER_ROOT = path.join(MODULE_ROOT, 'server') -const BUILTINS = new Set([...builtinModules, ...builtinModules.map((m) => `node:${m}`)]) - // Dependencies this half is allowed to resolve for itself. Empty, and that is // the design: everything the server half needs comes from `ctx` (§2.3). A new // entry here is a real decision — it becomes a package an operator's install @@ -148,7 +152,10 @@ function scan(root, moduleRoot = MODULE_ROOT, { shipped = isShipped, dev = devDe ? specifier.split('/').slice(0, 2).join('/') : specifier.split('/')[0] const allowed = ALLOWED_PACKAGES.has(pkg) || (!shipped(file) && dev.has(pkg)) - if (!BUILTINS.has(specifier) && !BUILTINS.has(pkg) && !allowed) { + // The `node:` prefix can only ever name a builtin, so it never reaches + // node_modules and is safe whatever this Node version enumerates. + const builtin = isBuiltin(specifier) || specifier.startsWith('node:') + if (!builtin && !allowed) { violations.push({ file, specifier, why: 'undeclared bare specifier — should this come from ctx?' }) } } diff --git a/server/test/checkImports.test.js b/server/test/checkImports.test.js index 6f6c674..5d0c8a1 100644 --- a/server/test/checkImports.test.js +++ b/server/test/checkImports.test.js @@ -81,6 +81,19 @@ test('allows node builtins anywhere, with or without the node: prefix', () => { ) }) +test('allows node:test, which older Node versions omit from builtinModules', () => { + // The first CI run failed on exactly this and on nothing else: `builtinModules` + // omits `test` on Node 20 and includes it on Node 24, so every test file in + // this suite was reported as breaking the module boundary. The check asks + // Node (`isBuiltin`) rather than rebuilding the list, and treats the `node:` + // prefix as sufficient on its own — a prefixed specifier can never resolve to + // a package, whatever the running version enumerates. + assert.deepStrictEqual( + scanFixture({ 'a.js': `require('node:test'); require('node:test/reporters')` }), + [], + ) +}) + test('catches ESM and dynamic forms, not only require()', () => { const found = scanFixture({ 'a.js': [`import db from '../../core/db.js'`, `const x = await import('../../core/other.js')`].join('\n'),