fix(middleware): replace p-limit with in-house limiter to fix Metro CJS load - #393
Merged
Conversation
…o CJS load p-limit is ESM-only since v3, but Metro loads @rozenite/middleware's plugin discovery through its CJS build (dist/index.cjs), which throws ERR_REQUIRE_ESM on Node versions without require(esm) interop. Swap it for a small local concurrency limiter with no external dependency. Also add an agent rule requiring Metro-related changes to be verified by actually running Metro in apps/playground, since this failure mode only shows up at runtime, not in unit tests or typecheck.
AGENTS.md should stay routing-only; the actual guidance belongs in docs/agents/ alongside the other testing guides.
…ests Move the bounded-concurrency limiter added to replace p-limit out of auto-discovery.ts into a shared, properly tested utility. Covers the behaviors that matter: the concurrency cap is enforced, queued work starts as slots free up, each call resolves with its own result, and a rejection propagates without breaking the rest of the queue.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Add a vitest test that spawns a real node subprocess (bypassing Vitest's own module runner, which prefers each package's `development` export condition and would otherwise mask the failure) to require the built @rozenite/tools, @rozenite/middleware, and @rozenite/metro CJS bundles and run withRozenite() end-to-end. This reproduces exactly how Metro loads these packages and would have caught the p-limit ESM/CJS break at require-time instead of only in production. Also note the test in docs/agents/metro-testing.md alongside the existing guidance to verify Metro-related changes against a real Metro boot.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@rozenite/middlewareusingp-limit.p-limithas been ESM-only since v3 (pinned here at^7.3.1), but Metro loads this package through its CJS build (dist/index.cjs), which doesrequire("p-limit"). On Node versions withoutrequire(esm)interop (pre-20.19) this throwsERR_REQUIRE_ESMand breaks Metro startup entirely; on newer Node it silently works, which is what let this ship unnoticed.createLimiter) local toauto-discovery.ts, removing the ESM/CJS boundary risk altogether.AGENTS.md: Metro-related changes must be verified by actually running Metro inapps/playground, since this class of failure only shows up at runtime and isn't caught by unit tests or typecheck.Test plan
pnpm --filter @rozenite/middleware run build— verifieddist/index.cjsanddist/index.jsno longer referencep-limitpnpm --filter @rozenite/middleware run typecheckpnpm --filter @rozenite/middleware run test(81 tests passing)CI=1 pnpm --filter playground start— Metro boots cleanly, all 14 plugins discovered, no ESM/require errors