diff --git a/AGENTS.md b/AGENTS.md index 844c9a86..39e6429a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,6 +14,7 @@ - For welcome dialog release content, see @./agents/release-content.md. - When working on `@rozenite/ui`, follow @./agents/working-on-ui-components.md. - Before preparing or opening a pull request, see @./docs/agents/pull-requests.md. +- For testing Metro-related changes, see @./docs/agents/metro-testing.md. - Preserve unrelated work already present in the working tree. - Keep changes focused; do not make opportunistic refactors. - Never commit credentials, secrets, generated build output, or local diff --git a/docs/agents/metro-testing.md b/docs/agents/metro-testing.md new file mode 100644 index 00000000..6838392f --- /dev/null +++ b/docs/agents/metro-testing.md @@ -0,0 +1,20 @@ +# Metro testing + +- Metro-related changes (Metro config, middleware loaded by Metro, plugin + discovery, anything reachable from `metro.config.js`) must be verified by + actually running Metro in `apps/playground` (e.g. `CI=1 pnpm --filter + playground start`), not just unit tests — Metro loads these packages as + CJS, and build/test suites won't catch runtime failures like requiring an + ESM-only dependency. +- Watch Metro's startup log for errors (e.g. `ERR_REQUIRE_ESM`) and confirm + it reaches `Waiting on http://localhost:8081` before concluding the change + is safe. +- `packages/metro/src/__tests__/cjs-load.test.ts` automates the require-time + half of this check: it spawns a real `node` subprocess (not Vitest's own + module runner, which prefers each package's `development` export + condition and would silently mask the failure) to `require()` the built + `@rozenite/tools`/`@rozenite/middleware`/`@rozenite/metro` CJS bundles and + run `withRozenite()` end-to-end, exactly as Metro does. Run it with + `pnpm --filter @rozenite/metro run test` after building the packages it + covers. It won't catch config-shape or bundling-time issues, so it doesn't + replace running Metro for real in `apps/playground`. diff --git a/packages/metro/package.json b/packages/metro/package.json index 0a627588..1f6d9ae3 100644 --- a/packages/metro/package.json +++ b/packages/metro/package.json @@ -42,7 +42,8 @@ "scripts": { "build": "vite build", "typecheck": "tsc -p tsconfig.lib.json --noEmit", - "lint": "eslint ." + "lint": "eslint .", + "test": "vitest --run --passWithNoTests" }, "dependencies": { "@rozenite/middleware": "workspace:*", @@ -51,7 +52,8 @@ "tslib": "^2.3.0" }, "devDependencies": { - "@react-native/metro-config": "~0.86.0" + "@react-native/metro-config": "~0.86.0", + "vitest": "^4.0.18" }, "engines": { "node": ">=20" diff --git a/packages/metro/src/__tests__/cjs-load.test.ts b/packages/metro/src/__tests__/cjs-load.test.ts new file mode 100644 index 00000000..7af33a90 --- /dev/null +++ b/packages/metro/src/__tests__/cjs-load.test.ts @@ -0,0 +1,62 @@ +import { execFileSync } from 'node:child_process'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; + +// Metro loads Rozenite through a plain `require()` (see +// apps/playground/metro.config.js), resolving each package's `require` +// export condition to its built CJS bundle (dist/index.cjs) -- never the +// TypeScript source. A dependency that's ESM-only (like p-limit was) can +// pass every other test here and still crash Metro at startup, because +// Vite/Rollup externalizes such dependencies into a literal top-level +// `require(...)` call in that bundle. +// +// This has to run in a real, separate `node` process rather than calling +// require()/createRequire() directly from this test: Vitest's own module +// runner intercepts module resolution for the whole worker (even for +// dynamic requires) and prefers each package's `development` export +// condition, which points at the TypeScript source -- silently bypassing +// the exact failure mode this test exists to catch. +// See docs/agents/metro-testing.md. +const packageRoot = path.resolve(fileURLToPath(import.meta.url), '../../..'); +const playgroundRoot = path.resolve(packageRoot, '../../apps/playground'); + +const runInNode = (script: string): string => + execFileSync(process.execPath, ['--input-type=module', '-e', script], { + cwd: packageRoot, + encoding: 'utf8', + timeout: 30_000, + }); + +describe('CJS load (mirrors how Metro requires Rozenite)', () => { + it('requires @rozenite/tools, @rozenite/middleware and @rozenite/metro without throwing', () => { + expect(() => + runInNode(` + import { createRequire } from 'node:module'; + const require = createRequire(import.meta.url); + require('@rozenite/tools'); + require('@rozenite/middleware'); + const mod = require('@rozenite/metro'); + if (typeof mod.withRozenite !== 'function') { + throw new Error('withRozenite was not exported as a function'); + } + `), + ).not.toThrow(); + }); + + it('runs withRozenite end-to-end (incl. plugin auto-discovery) under a real require()', () => { + const output = runInNode(` + import { createRequire } from 'node:module'; + const require = createRequire(import.meta.url); + const { withRozenite } = require('@rozenite/metro'); + const config = withRozenite({ projectRoot: ${JSON.stringify(playgroundRoot)} }, { enabled: true }); + const resolved = await config(); + console.log('__RESULT__' + JSON.stringify({ projectRoot: resolved.projectRoot })); + `); + + const resultLine = output.split('\n').find((line) => line.startsWith('__RESULT__')); + expect(JSON.parse(resultLine?.slice('__RESULT__'.length) ?? '')).toEqual({ + projectRoot: playgroundRoot, + }); + }); +}); diff --git a/packages/middleware/package.json b/packages/middleware/package.json index 2fc29975..f0a39f1a 100644 --- a/packages/middleware/package.json +++ b/packages/middleware/package.json @@ -50,7 +50,6 @@ "@rozenite/shell": "workspace:*", "@rozenite/tools": "workspace:*", "express": "^5.1.0", - "p-limit": "^7.3.1", "semver": "^7.7.2", "tslib": "^2.3.0", "ws": "^8.18.3" diff --git a/packages/middleware/src/auto-discovery.ts b/packages/middleware/src/auto-discovery.ts index a7de6186..fbf57b24 100644 --- a/packages/middleware/src/auto-discovery.ts +++ b/packages/middleware/src/auto-discovery.ts @@ -3,7 +3,7 @@ import os from 'node:os'; import path from 'node:path'; import assert from 'node:assert'; import { createRequire } from 'node:module'; -import pLimit from 'p-limit'; +import { createLimiter } from '@rozenite/tools'; import { logger } from './logger.js'; import { ROZENITE_MANIFEST } from './constants.js'; import { RozeniteConfig } from './config.js'; @@ -252,7 +252,7 @@ const getInstalledPluginsFromDependencies = async ( (dependency) => !options.exclude?.includes(dependency), ); - const limit = pLimit(DISCOVERY_CONCURRENCY); + const limit = createLimiter(DISCOVERY_CONCURRENCY); const plugins = await Promise.all( dependencies.map((dependency) => limit(() => resolveInstalledPlugin(options.projectRoot, dependency)), diff --git a/packages/tools/package.json b/packages/tools/package.json index 2b7ba335..a9a11d20 100644 --- a/packages/tools/package.json +++ b/packages/tools/package.json @@ -28,11 +28,13 @@ "scripts": { "build": "vite build", "typecheck": "tsc -p tsconfig.lib.json --noEmit", - "lint": "eslint ." + "lint": "eslint .", + "test": "vitest --run --passWithNoTests" }, "devDependencies": { "metro-config": "*", "typescript": "^5.7.3", - "vite": "catalog:" + "vite": "catalog:", + "vitest": "^4.0.18" } } diff --git a/packages/tools/src/__tests__/limiter.test.ts b/packages/tools/src/__tests__/limiter.test.ts new file mode 100644 index 00000000..47372983 --- /dev/null +++ b/packages/tools/src/__tests__/limiter.test.ts @@ -0,0 +1,99 @@ +import { describe, it, expect } from 'vitest'; +import { createLimiter } from '../limiter.js'; + +const deferred = () => { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +}; + +describe('createLimiter', () => { + it('never runs more tasks concurrently than the given limit', async () => { + const limit = createLimiter(2); + let active = 0; + let maxActive = 0; + const gates = Array.from({ length: 6 }, () => deferred()); + + const runs = gates.map((gate, i) => + limit(async () => { + active++; + maxActive = Math.max(maxActive, active); + await gate.promise; + active--; + return i; + }), + ); + + // Let the microtask queue settle so the first batch actually starts. + await Promise.resolve(); + await Promise.resolve(); + expect(maxActive).toBe(2); + + gates.forEach((gate) => gate.resolve()); + const results = await Promise.all(runs); + expect(results).toEqual([0, 1, 2, 3, 4, 5]); + expect(maxActive).toBe(2); + }); + + it('starts queued work as running slots free up', async () => { + const limit = createLimiter(1); + const order: number[] = []; + const gate = deferred(); + + const first = limit(async () => { + await gate.promise; + order.push(1); + }); + const second = limit(async () => { + order.push(2); + }); + + await Promise.resolve(); + expect(order).toEqual([]); // second hasn't started yet, first still holds the slot + + gate.resolve(); + await Promise.all([first, second]); + expect(order).toEqual([1, 2]); + }); + + it('resolves each call with its own task result', async () => { + const limit = createLimiter(3); + const results = await Promise.all([ + limit(async () => 'a'), + limit(async () => 'b'), + limit(async () => 'c'), + ]); + expect(results).toEqual(['a', 'b', 'c']); + }); + + it('propagates a rejected task to its own caller without affecting others', async () => { + const limit = createLimiter(2); + const failure = new Error('boom'); + + const results = await Promise.allSettled([ + limit(async () => { + throw failure; + }), + limit(async () => 'ok'), + ]); + + expect(results[0]).toEqual({ status: 'rejected', reason: failure }); + expect(results[1]).toEqual({ status: 'fulfilled', value: 'ok' }); + }); + + it('keeps processing the queue after an earlier task rejects', async () => { + const limit = createLimiter(1); + + const first = limit(async () => { + throw new Error('boom'); + }); + const second = limit(async () => 'still runs'); + + await expect(first).rejects.toThrow('boom'); + await expect(second).resolves.toBe('still runs'); + }); +}); diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index 2a089e5b..43eaa904 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -7,6 +7,7 @@ export { UnknownBundlerType, } from './project-type.js'; export { logger } from './logger.js'; +export { createLimiter, type Limiter } from './limiter.js'; export { createMetroConfigTransformer, composeMetroConfigTransformers, diff --git a/packages/tools/src/limiter.ts b/packages/tools/src/limiter.ts new file mode 100644 index 00000000..6d8cfd5a --- /dev/null +++ b/packages/tools/src/limiter.ts @@ -0,0 +1,31 @@ +export type Limiter = (task: () => Promise) => Promise; + +// Metro loads this package as CJS, so callers can't depend on p-limit +// (ESM-only since v3) without risking `ERR_REQUIRE_ESM` on Node versions +// that don't support requiring ESM. This owns the same bounded-concurrency +// semantics without an external dependency. +export const createLimiter = (concurrency: number): Limiter => { + let active = 0; + const queue: (() => void)[] = []; + + const next = () => { + if (active >= concurrency || queue.length === 0) { + return; + } + active++; + queue.shift()?.(); + }; + + return (task: () => Promise): Promise => + new Promise((resolve, reject) => { + queue.push(() => { + task() + .then(resolve, reject) + .finally(() => { + active--; + next(); + }); + }); + next(); + }); +}; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index edcbdfc1..b1c09493 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -776,6 +776,9 @@ importers: '@react-native/metro-config': specifier: ~0.86.0 version: 0.86.2(@babel/core@7.29.0(supports-color@8.1.1))(supports-color@8.1.1) + vitest: + specifier: ^4.0.18 + version: 4.1.0(@types/node@18.16.9)(@vitest/ui@3.2.4(vitest@3.2.4))(jiti@2.4.2)(jsdom@22.1.0(supports-color@8.1.1))(lightningcss@1.32.0)(terser@5.43.1)(tsx@4.21.0)(yaml@2.8.1) packages/middleware: dependencies: @@ -794,9 +797,6 @@ importers: express: specifier: ^5.1.0 version: 5.1.0(supports-color@8.1.1) - p-limit: - specifier: ^7.3.1 - version: 7.3.1 semver: specifier: ^7.7.2 version: 7.7.2 @@ -1642,6 +1642,9 @@ importers: vite: specifier: ^7.3.1 version: 7.3.1(@types/node@18.16.9)(jiti@2.4.2)(lightningcss@1.32.0)(terser@5.43.1)(tsx@4.21.0)(yaml@2.8.1) + vitest: + specifier: ^4.0.18 + version: 4.1.0(@types/node@18.16.9)(@vitest/ui@3.2.4(vitest@3.2.4))(jiti@2.4.2)(jsdom@22.1.0(supports-color@8.1.1))(lightningcss@1.32.0)(terser@5.43.1)(tsx@4.21.0)(yaml@2.8.1) packages/ui: dependencies: @@ -12399,10 +12402,6 @@ packages: resolution: {integrity: sha512-7cIXg/Z0M5WZRblrsOla88S4wAK+zOQQWeBYfV3qJuJXMr+LnbYjaadrFaS0JILfEDPVqHyKnZ1Z/1d6J9VVUw==} engines: {node: '>=20'} - p-limit@7.3.1: - resolution: {integrity: sha512-0trZaiG7Y7kN/Egy9a8j47t9osC0Tch4PaIWd9yGF6bvmlk7muExRvGNYb8sXBwEKMoNKsbNN9P8EefuQekE4Q==} - engines: {node: '>=20'} - p-locate@3.0.0: resolution: {integrity: sha512-x+12w/To+4GFfgJhBEpiDcLozRJGegY+Ei7/z0tSLkMmxGZNybVMSfWj9aJn8Z5Fc7dBUNJOOVgPv2H7IwulSQ==} engines: {node: '>=6'} @@ -29764,10 +29763,6 @@ snapshots: dependencies: yocto-queue: 1.2.1 - p-limit@7.3.1: - dependencies: - yocto-queue: 1.2.1 - p-locate@3.0.0: dependencies: p-limit: 2.3.0 @@ -32792,11 +32787,11 @@ snapshots: magic-string: 0.30.21 obug: 2.1.1 pathe: 2.0.3 - picomatch: 4.0.3 + picomatch: 4.0.4 std-env: 4.0.0 tinybench: 2.9.0 tinyexec: 1.0.4 - tinyglobby: 0.2.15 + tinyglobby: 0.2.17 tinyrainbow: 3.0.3 vite: 7.3.5(@types/node@18.16.9)(jiti@2.4.2)(lightningcss@1.32.0)(terser@5.43.1)(tsx@4.21.0)(yaml@2.8.1) why-is-node-running: 2.3.0