diff --git a/.changeset/20376-plugin-dev-literal-app-imports.md b/.changeset/20376-plugin-dev-literal-app-imports.md new file mode 100644 index 00000000000..2222d11cea1 --- /dev/null +++ b/.changeset/20376-plugin-dev-literal-app-imports.md @@ -0,0 +1,11 @@ +--- +"@objectstack/plugin-dev": patch +--- + +`DevPlugin` now loads `@objectstack/setup` and `@objectstack/account` through literal `import('…')` specifiers, like every other declared dependency it loads, instead of one variable specifier shared by a loop (#20376). + +Clause-②: no + +- **What was wrong.** The setup / account app-package loop imported `spec[0]`. A variable specifier cannot be resolved when the file is transformed. Under vitest, every `DevPlugin.init()` in a test therefore made two round trips to the main test process, even with both packages mocked, inside every clocked test window that boots `DevPlugin`. The main process is shared by the whole run, so on a busy CI shard those round trips wait on other files' work, against this package's 5000 ms test budget. +- **What changes.** Each loop entry carries its own literal loader. The `try` / `catch` and the absent-package report around each load are unchanged: a missing package is still logged as, for example, `✘ @objectstack/setup not installed — skipping its app`, and a present one that fails is still reported as present-but-failed. +- **Unchanged.** Nothing an author or operator configures or sees changes. Both packages were already declared dependencies of `@objectstack/plugin-dev`, and both the ESM and the CJS build keep a native `import("…")` for each. diff --git a/packages/plugins/plugin-dev/src/dev-plugin-literal-imports.pin.test.ts b/packages/plugins/plugin-dev/src/dev-plugin-literal-imports.pin.test.ts new file mode 100644 index 00000000000..5c34efb53e4 --- /dev/null +++ b/packages/plugins/plugin-dev/src/dev-plugin-literal-imports.pin.test.ts @@ -0,0 +1,114 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #20376 — every dynamic `import()` in `dev-plugin.ts` names its package +// LITERALLY, except the one ADR-0132 requires to stay a variable. +// +// ## Why the FORM of the specifier is pinned +// +// Every suite in this family mocks the packages `DevPlugin.init()` loads, so +// `init()` should run in-process. For a literal `import('x')` it does: the +// specifier is rewritten when `dev-plugin.ts` is transformed, and the test's +// `vi.mock('x')` serves it inside the worker. A VARIABLE specifier is resolved +// at CALL time instead, by a round trip to the main vitest process, and that +// happens on every call, not only the first. Measured with a throwaway vite +// plugin logging every `resolveId` and `transform` the main process served, +// around six back-to-back `init()`s with setup and account mocked absent: +// +// - With the loop importing through `spec[0]` (`dev-plugin.ts` as of +// `0fcb10184`), the six `init()`s made the main process serve 20 +// requests: a `resolveId` of `@objectstack/setup` and one of +// `@objectstack/account` on EVERY `init()`, and on the first one also the +// transform of both packages' `dist` entries and the resolution of their +// imports. The mount-refusal suite's nine `init()`s made 18 of those +// `resolveId`s. +// - With literal specifiers, the same six `init()`s made ZERO. Every +// specifier this file names, the twelve literal loads beside the loop +// included, is resolved once, when the file is transformed at collection. +// - The main process is one process shared by every worker in the run, so +// a request to it waits on whatever it is doing for OTHER files. On +// `Test Core (6/6)` (job 108779589417), the mount-refusal suite's first +// case timed out at 5026 ms and its second took 2336 ms, while its other +// four took 16-72 ms, in a package run that read `transform 74.29s, +// import 137.46s, tests 11.87s`. +// +// So re-introducing a variable specifier puts a main-process round trip back +// into every clocked window that boots this plugin, whatever the suite. A +// timeout raised to absorb it would only move the cliff to the next heavier +// shard. +// +// ## The one exception +// +// `@objectstack/organizations` is imported through `organizationsPkg`, with +// `webpackIgnore`, on purpose: ADR-0132's entitlement boundary forbids any +// framework package from declaring it (`no-framework-dependents.pin.test.ts`), +// so no bundler may try to resolve it. In the suites that mock it, its mock +// URL is the raw specifier, so the worker serves it without a round trip. +// +// ## How it reads the file +// +// Through the TypeScript compiler's own parser, never through the source text: +// a dynamic import is a call node whose callee is the `import` keyword, so a +// comment, a string or a regex that mentions `import(` is never read as one, +// and the leading `/* webpackIgnore: true */` is trivia, not part of the +// argument. The lit control (③) proves the walk still finds the literal +// loads, so a walk that found nothing cannot pass ① vacuously. + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import ts from 'typescript'; + +// `__dirname`, not `import.meta.url`: this package's build config compiles +// `src/**` as CommonJS, where `import.meta` is TS1470. Vitest's evaluator +// provides `__dirname` to every module it runs. +const HERE = __dirname; +const FILE = resolve(HERE, 'dev-plugin.ts'); +const SOURCE = ts.createSourceFile(FILE, readFileSync(FILE, 'utf8'), ts.ScriptTarget.Latest, true); + +/** Every node of the parsed file, depth first. */ +function nodesOf(root: ts.Node): ts.Node[] { + const out: ts.Node[] = []; + const visit = (node: ts.Node): void => { + out.push(node); + ts.forEachChild(node, visit); + }; + visit(root); + return out; +} + +const NODES = nodesOf(SOURCE); + +/** The first argument of every dynamic `import(…)` call in the file. */ +const DYNAMIC_IMPORT_ARGS: ts.Expression[] = NODES + .filter((n): n is ts.CallExpression => ts.isCallExpression(n) && n.expression.kind === ts.SyntaxKind.ImportKeyword) + .map((call) => call.arguments[0]); + +/** + * Only a plain string literal counts as naming its package: it is the form the + * transform resolves ahead of time. Anything else is resolved at call time. + */ +const LITERAL_ARGS = DYNAMIC_IMPORT_ARGS.filter(ts.isStringLiteral).map((arg) => arg.text); +const VARIABLE_ARGS = DYNAMIC_IMPORT_ARGS.filter((arg) => !ts.isStringLiteral(arg)).map((arg) => arg.getText(SOURCE)); + +describe('#20376 — dev-plugin.ts loads its packages through literal specifiers', () => { + it('① every dynamic import names its package literally, except the organizations one', () => { + expect(VARIABLE_ARGS, 'a variable specifier costs a main-process round trip per call under vitest').toEqual([ + 'organizationsPkg', + ]); + }); + + it('② the exception is the ADR-0132 package, and nothing else hides behind that name', () => { + const declarations = NODES.filter( + (n): n is ts.VariableDeclaration => ts.isVariableDeclaration(n) && n.name.getText(SOURCE) === 'organizationsPkg', + ); + expect(declarations).toHaveLength(1); + const init = declarations[0].initializer; + expect(init !== undefined && ts.isStringLiteral(init) ? init.text : undefined).toBe('@objectstack/organizations'); + }); + + it('③ lit control: the walk sees the setup / account loads, and the loads beside them, as literals', () => { + for (const pkg of ['@objectstack/setup', '@objectstack/account', '@objectstack/objectql']) { + expect(LITERAL_ARGS, pkg).toContain(pkg); + } + }); +}); diff --git a/packages/plugins/plugin-dev/src/dev-plugin-optional-load-failure.test.ts b/packages/plugins/plugin-dev/src/dev-plugin-optional-load-failure.test.ts index 799d9dbe7fd..77cb76cb9ec 100644 --- a/packages/plugins/plugin-dev/src/dev-plugin-optional-load-failure.test.ts +++ b/packages/plugins/plugin-dev/src/dev-plugin-optional-load-failure.test.ts @@ -285,4 +285,24 @@ describe('DevPlugin — an optional service that is installed and fails to const // And the false claim it used to emit instead is gone. expect(allLines(ctx).some((l) => l.includes('@objectstack/rest not installed'))).toBe(false); }); + + // #20376 changed HOW the setup / account app-package loop names its two + // packages (a literal `import()` per entry, in place of one variable + // specifier), never what it does when one cannot be imported. Both still + // reach the absent arm: one line each, at the loop's `warn`, never at the + // present-but-failed arm's `error`, and carrying the resolver's own code and + // the specifier that failed. + it('keeps the setup / account app packages on the absent arm when they cannot be imported', async () => { + const ctx = mockCtx(); + await new DevPlugin({ seedAdminUser: false }).init(ctx); + + const warnLines: string[] = ctx.logger.warn.mock.calls.map((call: unknown[]) => String(call[0])); + for (const pkg of ['@objectstack/setup', '@objectstack/account']) { + const lines = warnLines.filter((l) => l.includes(pkg)); + expect(lines, `${pkg}: reported once, at warn`).toHaveLength(1); + expect(lines[0]).toContain('code: ERR_MODULE_NOT_FOUND'); + expect(lines[0]).toContain(`Cannot find package '${pkg}'`); + expect(errorLines(ctx).some((l) => l.includes(pkg)), `${pkg}: not the present-but-failed arm`).toBe(false); + } + }); }); diff --git a/packages/plugins/plugin-dev/src/dev-plugin.ts b/packages/plugins/plugin-dev/src/dev-plugin.ts index b132ad3f467..1f1ac117f46 100644 --- a/packages/plugins/plugin-dev/src/dev-plugin.ts +++ b/packages/plugins/plugin-dev/src/dev-plugin.ts @@ -734,12 +734,21 @@ export class DevPlugin implements Plugin { // NOTE: @objectstack/studio is intentionally NOT default-loaded — the // console ships a dedicated Studio surface at /_console/studio//, // so Studio no longer needs to exist as a navigable app tile. + // + // [#20376] Each entry carries its own LITERAL `import('…')`, as every + // other declared-dependency load in this method does. A variable + // specifier (`import(spec[0])`) cannot be resolved when this file is + // transformed: under vitest every call became a round trip to the main + // process — two per `init()`, measured, mocked packages included — + // inside every clocked test window that boots this plugin. Both packages + // are declared dependencies, and the absent-package path below is + // unchanged. `dev-plugin-literal-imports.pin.test.ts` pins the form. for (const spec of [ - ['@objectstack/setup', 'createSetupAppPlugin'], - ['@objectstack/account', 'createAccountAppPlugin'], + ['@objectstack/setup', 'createSetupAppPlugin', () => import('@objectstack/setup')], + ['@objectstack/account', 'createAccountAppPlugin', () => import('@objectstack/account')], ] as const) { try { - const mod: any = await import(/* @vite-ignore */ spec[0]); + const mod: any = await spec[2](); this.childPlugins.push(mod[spec[1]]()); ctx.logger.info(` ✔ App package enabled (${spec[0]})`); } catch (err) { diff --git a/packages/plugins/plugin-dev/vitest.config.ts b/packages/plugins/plugin-dev/vitest.config.ts index a8835104050..d619c52a005 100644 --- a/packages/plugins/plugin-dev/vitest.config.ts +++ b/packages/plugins/plugin-dev/vitest.config.ts @@ -62,6 +62,14 @@ export default defineConfig({ }, { find: /^@objectstack\/platform-objects$/, replacement: path.resolve(__dirname, '../../platform-objects/src/index.ts') }, { find: /^@objectstack\/plugin-security$/, replacement: path.resolve(__dirname, '../plugin-security/src/index.ts') }, + // [#20376] The setup / account app packages. `dev-plugin.ts` names them with + // literal `import('…')` specifiers now, so `check:test-source-alias` sees + // both and, unaliased, both resolve through `exports` to `dist/`. Every + // suite here mocks them; the mocks and the imports resolve through these + // same entries. Their own imports (`@objectstack/platform-objects/apps`, + // `@objectstack/spec/system`) are already aliased above and below. + { find: /^@objectstack\/setup$/, replacement: path.resolve(__dirname, '../../apps/setup/src/index.ts') }, + { find: /^@objectstack\/account$/, replacement: path.resolve(__dirname, '../../apps/account/src/index.ts') }, { find: /^@objectstack\/formula$/, replacement: path.resolve(__dirname, '../../formula/src/index.ts') }, { find: /^@objectstack\/metadata-core$/, replacement: path.resolve(__dirname, '../../metadata-core/src/index.ts') }, // Subpath BEFORE the bare package: `@objectstack/core` is a PREFIX match with a