fix(build): expand require.context into import.meta.glob map (#1501)#1836
Conversation
|
/bigbonk review for issues |
commit: |
|
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
/bigbonk reviewReviewed the Bug: stateful regex flags (
|
|
Posted the review on PR #1836. Summary of findings: Bug found — stateful regex flags ( Note — Minor — misleading comment in Confirmed correct: TS-wrapper unwrapping, relative-path guard (incl. |
…ve to true Address ask-bonk review on #1836: - Strip global/sticky flags when reconstructing the filter regexp; a stateful g/y RegExp.test() drops every other matching module. - Default the omitted recursive arg to true to match Webpack's useSubdirectories default (was silently false). - Add a g-flagged fixture/test case locking in the flag-stripping fix. - Clarify the misleading descent comment in collectRequireContextCalls.
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
/bigbonk reviewRe-reviewed after Resolved since last review
Remaining notes (non-blocking)
Confirmed correct
LGTM once the |
|
Posted the review on PR #1836. Summary: The two correctness bugs from the prior
Remaining notes (non-blocking, coverage/docs only):
No correctness regressions found in the current revision. The transform is conservative and matches Next.js/Webpack behavior. |
Address ask-bonk follow-up: lock in the runtime contract beyond .keys().
- Assert ctx('./parent/file1.js').default resolves the module namespace.
- Assert an unknown key throws with code 'MODULE_NOT_FOUND'.
|
/bigbonk review for issues |
/bigbonk reviewRe-reviewed at the current tip ( Verified resolved
Independent verification
Non-blocking notes (parity, low-risk)
LGTM. The transform is appropriately conservative (literal-only matching, TS-wrapper unwrapping, relative-path guard, cheap substring gate, parse-failure fall-through), and the two real bugs surfaced earlier are fixed and regression-tested. |
|
Posted the review on PR #1836. Summary: I re-reviewed at the current tip ( Verified resolved:
Independent verification:
Non-blocking notes (parity, low-risk):
No correctness regressions. LGTM. |
Address ask-bonk note: with no filter regexp, import.meta.glob only surfaces resolvable modules, so extensionless keys Webpack would include can be dropped. Documented as a low-risk divergence (upstream Next.js's extensionless test is disabled/Turbopack-pending).
|
/bigbonk review for issues |
/bigbonk reviewRe-reviewed at the current tip ( Independently verified this round
Non-blocking notes (low severity, no change required)
LGTM. The transform is appropriately conservative (literal-only matching, TS-wrapper unwrapping, relative-path guard, parse-failure fall-through), the two real bugs from earlier rounds are fixed and regression-tested, and the runtime contract ( |
|
Posted the review on PR #1836. SummaryI re-reviewed at the current tip ( Independently verified this round:
Non-blocking notes (no change required):
Verdict: LGTM. The transform is conservative and correct; both real bugs from earlier rounds are fixed and regression-tested, and the runtime contract is now fully covered by tests. |
Problem
require.context(dir, recursive, regexp)is a Webpack-only build-time API that builds a static module map. Next.js apps still use it — typically written(require as any).context(...)so it type-checks. Under vinext (Vite/Rolldown) there is no such API, so the call throwsrequire is not definedat runtime and the page fails to render.Failing upstream suite:
test/e2e/app-dir/require-context.Fix
Adds a small build-time transform plugin (
packages/vinext/src/plugins/require-context.ts, registered inindex.ts) that rewrites each genuinerequire.context(...)call into an IIFE backed by Vite'simport.meta.glob(..., { eager: true }). The IIFE exposes the subset of the Webpack context interface used in practice:ctx.keys()— sorted keys relative to the base dir (e.g../parent/file1.js)ctx(key)— the module namespace object (throwsMODULE_NOT_FOUNDfor unknown keys)ctx.resolve(key)/ctx.id— best-effortScope / safety
enforce: "pre"so the original(require as any).context(...)form is still visible (the callee object is aTSAsExpression). The matcher unwrapsTSAsExpression/TSSatisfiesExpression/TSNonNullExpression/ parenthesized wrappers down to the barerequireidentifier../or../), an optional literal booleanrecursive, and an optional regexp literal. Anything dynamic or an unrelated.context(...)call is left untouched.require+.context) before parsing.Test
Ported the upstream regex-filtering case into
tests/nextjs-compat/require-context.test.tswith a minimal fixture undertests/fixtures/app-basic/app/nextjs-compat/require-context/. Assertskeys()returns['./parent/file1.js', './parent/file2.js', './parent2/file3.js'].Closes #1501