fix(build): apply configured webpack loaders#2348
Conversation
|
/bigbonk review for issues |
commit: |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: apply configured webpack loaders
Solid, well-scoped approach. The shim runs matching loader chains via a \0-prefixed virtual module and correctly defers to it ahead of static-image handling. I verified the new test passes and ran a few probes. A few correctness/perf items worth addressing before merge.
1. Loaders run multiple times per resource (probe + once per environment) — non-idempotent side effects will fire repeatedly
The loader pipeline now runs once during the config probe (invokeLoaderSideEffects) and again in load() once per Vite environment (RSC, SSR, client). I confirmed this with an instrumented loader: a single .svg import imported from a server component invoked the loader 3 times (1 probe + RSC + SSR). For a module imported from a client component it would be 4 (probe + RSC + SSR + client).
INVOCATIONS:
rp=<none> # invokeLoaderSideEffects (probe)
rp=.../app/next.svg # load() in RSC env
rp=.../app/next.svg # load() in SSR env
invokeLoaderSideEffects exists specifically to capture process.env mutations at config time (issue #1500). Now the same loader's side effects run a second/third time during load(). For the env-mutation case this happens to be idempotent, but any loader that:
- appends to a file / increments a counter / emits an artifact,
- posts telemetry, or
- is simply expensive,
will do so once per environment plus once for the probe. Next.js runs each loader once per asset. Worth either deduping the work across environments (cache the transformed output by resourcePath) or at minimum documenting that loaders must be pure. The probe-then-transform double-run is the part most likely to surprise users.
2. resolveId runs this.resolve() for every import in every module when any loader rule is configured
async resolveId(source, importer) {
const configuredRules = getRules();
if (configuredRules.length === 0) return null;
if (source.startsWith(WEBPACK_LOADER_PREFIX) || source.startsWith("\0")) return null;
const resolved = await this.resolve(source, importer, { skipSelf: true });
...Once a single loader rule exists, this performs a full this.resolve() (an async resolver round-trip) for every non-virtual import across RSC/SSR/client graphs, then collectMatchingWebpackLoaderRules on each. Per AGENTS.md "Performance Best Practices" (filter hooks before JS runs, keep the common path light), consider a native hook filter so the JS handler only fires for candidate ids. If the configured rules' test conditions are extension-based (the overwhelmingly common case — /\.svg$/, etc.), you can derive an include regex from the rule tests and attach it as a resolveId.filter.id, avoiding the resolver call for the bulk of imports. At minimum, gate on a cheap extension check of source before calling this.resolve().
3. emitError rejects the whole load — diverges from webpack (non-fatal)
emitError: reject,In webpack, this.emitError() records a diagnostic but does not abort the loader; compilation continues. Here it rejects the runLoader promise, which fails the build. A loader that calls emitError for a recoverable condition (and still returns output) would break under vinext but succeed under Next.js. Consider treating emitError like emitWarning (log) rather than reject, reserving rejection for thrown errors / callback(err).
4. Return value ignored vs. honored when this.async() is used
const result = loader.call(context, input);
if (result && typeof result.then === "function") { ... }
else if (result !== undefined) { complete(null, result); } // <-- races async()If a loader calls this.async() and then returns a non-undefined value (some loaders return undefined explicitly, but not all), line 166 completes immediately with the return value, racing the async callback. Webpack ignores the sync return value once this.async() has been requested. Guard with else if (!asyncRequested && result !== undefined).
5. Minimal loader context — common APIs missing
The LoaderContext omits frequently-used members: addDependency, addContextDependency, resourceQuery, getResolve, fs, sourceMap, data, loaders/loaderIndex, utils, target. A loader calling this.addDependency(...) (very common) throws this.addDependency is not a function, which under item #3's reject semantics fails the build. Even no-op stubs for the common ones would broaden compatibility for "narrow loader support." Worth a note in the doc comment listing what is/isn't provided.
6. Nested rules conditions are not ANDed with the parent
In collectMatchingWebpackLoaderRules, for a parent rule with rules: [...], children are visited with only their own conditions checked — the parent's test/include/exclude are not combined. Webpack ANDs ancestor conditions with descendant conditions. Edge case (most configs use flat rules or oneOf), but a latent divergence worth a comment or follow-up.
Test coverage suggestions
The single fixture test is a good port of the Next.js env-mutation case. Consider adding focused unit tests for the pure helpers (no build needed, fast):
collectMatchingWebpackLoaderRuleswithoneOf,exclude,include, nestedrules, and arrayuseorderingmatchesConditionfor RegExp / string-prefix / function / array /{ and, or, not }objects- A multi-loader chain asserting right-to-left execution order
These would lock in the matching semantics independently of the (slow) buildApp() path and guard against regressions in the condition logic.
None of these block the core fix (the env-var parity case works). Items #1, #2, and #3 are the ones I'd prioritize: #1 because non-idempotent loaders will misbehave, #2 for the per-import resolve cost on every app that configures any loader, and #3 for Next.js parity on emitError.
|
Review posted to PR #2348. Summary of findings: Review summary for PR #2348The core fix works — I ran the new test (passes) and instrumented the loader to verify runtime behavior. I flagged 6 issues, 3 of them prioritized: Prioritized:
Lower priority: Plus a suggestion to add fast unit tests for the pure helpers ( None block the core env-var parity fix. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: apply configured webpack loaders (follow-up)
The core env-var parity fix works and the new tests pass locally (tests/webpack-loader-compat.test.ts, 4/4). The prior review already covered the big items (multiple invocations per resource, resolveId resolve-on-every-import cost, emitError: reject parity, async-return race, minimal LoaderContext, nested-rule AND semantics). I won't repeat those. Below are distinct findings I confirmed by instrumenting the helpers.
1. resolveId strips the query string, then force-routes asset imports through the loader (?url / ?raw regression)
const resourcePath = resolved.id.split("?", 1)[0]; // query dropped
...
return `${WEBPACK_LOADER_PREFIX}${resourcePath}`; // query never restoredlet source: string | Buffer = await fs.readFile(resourcePath); // load() has no queryWith enforce: "pre", this hook runs ahead of Vite's asset pipeline. Any import like import url from "./icon.svg?url" or import raw from "./icon.svg?raw" whose path matches a configured rule is rewritten to \0vinext-webpack-loader:/abs/icon.svg, the ?url/?raw suffix is discarded, and the file is fed straight through the loader chain. The user loses Vite's normal ?url/?raw handling for that resource, and load() reads the file with no knowledge of the original query. This is a real behavioral divergence the moment a project has any extension-based rule (e.g. /\.svg$/) and also uses query-suffixed imports of the same extension. At minimum the query should be preserved through the prefixed id and load() should split it back off before fs.readFile.
2. resourceQuery / issuer rule conditions are ignored entirely
matchesRule only checks test / include / exclude. Webpack rules commonly key off resourceQuery (this is the idiomatic way to do ?url/?inline/?react variants). Confirmed:
collectMatchingWebpackLoaderRules([{ test: /\.svg$/, resourceQuery: /url/, use: ["l"] }], "/x/a.svg")
-> matched (resourceQuery ignored)
So a rule that should only apply to *.svg?react (e.g. SVGR's common config) is applied to every .svg, and a rule scoped to ?url is applied to bare imports. Combined with #1 (query dropped before matching), resourceQuery-scoped rules can't work at all. Worth either honoring resourceQuery or explicitly documenting that only path-based conditions are supported and bailing out when a matched rule carries a resourceQuery/issuer condition (so we don't silently mis-apply it).
3. isFrameworkLoader matches "mdx" as a bare substring — over-broad
loaderPath.includes("mdx")This skips any loader whose resolved path contains mdx anywhere, including a legitimate user loader at e.g. /app/loaders/mdx-frontmatter.cjs or a package like remark-mdx-foo. The next-babel-loader and next/dist/build/webpack checks are appropriately specific; the mdx check should be tightened (e.g. match @next/mdx / @mdx-js/loader specifically) so user loaders with "mdx" in the path aren't silently dropped. Note this same over-broad check is duplicated in next-config.ts's invokeLoaderSideEffects (lines ~1946-1948).
4. {} / empty-object conditions return false (webpack treats them as match)
collectMatchingWebpackLoaderRules([{ test: {}, use: ["l"] }], "/x/a.svg") -> no matchIn matchesCondition, an object with none of and/or/not falls through to return record.and !== undefined || record.or !== undefined || record.not !== undefined → false. Webpack treats an empty condition object as matching. Minor/edge, but a latent divergence worth a guard or comment.
5. oneOf parent with its own use reverses loader order
parent = { test: /\.(svg|png)$/, use: ["parent-loader"], oneOf: [ {test:/\.svg$/, use:["a-loader"]}, ... ] }
collectMatchingWebpackLoaderRules([parent], "/x/icon.svg")
-> [ ["a-loader"], ["parent-loader"] ]
The matched oneOf child is pushed before the parent (which still contributes its own use). After loaders.reverse() in load(), parent-loader runs before a-loader. Webpack's own normalization flattens these differently. This overlaps the prior review's nested-rules note (#6 there), but the concrete symptom here is wrong execution order when a oneOf parent also carries loaders, not just un-ANDed conditions. Low priority (unusual config), but worth a unit test pinning the intended order.
Priorities, from my list: #1 (query drop + force-routing) and #2 (resourceQuery ignored) are the ones most likely to bite real apps the moment any extension rule is configured alongside ?url/?raw/?react imports — exactly the SVGR-style setups this feature targets. #3 can silently drop a valid user loader. #4/#5 are edge cases / follow-ups.
None block the env-var parity fix that motivated the PR.
|
Review posted to PR #2348. SummaryI reviewed PR #2348 ( Prioritized (new):
Lower priority: I explicitly avoided repeating the prior review's items (multiple invocations, resolveId perf, |
|
/bigbonk review for issues |
|
Targeted independent review for the baseline The PR does cover the exact run-28143992598 failure and the fixture is faithful to the Next.js v16.2.6 setup. Please keep this draft for one remaining compatibility blocker before graduation:
Suggested focused remediation: carry the full resource query into matching/context and honor I also confirmed the async callback race reported earlier is fixed at current head |
|
/bigbonk review for issues |
|
Exact-head CI blocker at
The image transform unit harness invokes the plugin before Failed job: https://github.com/cloudflare/vinext/actions/runs/28185426521/job/83485969127 |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
There was a problem hiding this comment.
Review: apply configured webpack loaders (current head ff1d943)
I re-reviewed at the current head, ran the suite, and confirmed the prior review items have been addressed across the 8 commits:
- Query strings are now preserved end-to-end (resource + issuer base64url-encoded into the virtual id, decoded in
load();fs.readFilereads the path,resourceQueryflows to the loader context). Verified by the new "preserves resource queries for matching loaders" test. resourceQuery/issuerconditions are now honored inmatchesRule(webpack-loader-compat.ts:78-91), with a focused unit test.- The async-loader race is fixed — sync return is ignored once
this.async()is requested (webpack-loader-compat.ts:232), covered by "waits for async loader callbacks". isFrameworkLoader/isMdxLoaderno longer match a bare"mdx"substring;next-config.tsnow reusesisMdxLoader. Covered by "runs custom loaders whose paths contain mdx".emitErrorlogs instead of rejecting (webpack-loader-compat.ts:207), matching webpack's non-fatal semantics.- Nested
rulesnow AND the parent conditions viaparentMatches(webpack-loader-compat.ts:100-122), with a regression test. - The exact-head
image-imports.test.tscrash (Cannot read properties of undefined (reading 'webpackLoaderRules')) is fixed vianextConfig?.webpackLoaderRules...optional chaining (index.ts:4810-4811) — all 27 tests pass. - Fast unit tests for the pure helpers were added (the file is now 9 tests, not 4).
Local verification (all green):
tests/webpack-loader-compat.test.ts— 9/9tests/image-imports.test.ts— 27/27tests/next-config.test.ts -t webpack— 11/11tests/build-optimization.test.ts— 149/149vp checkon the four changed source/test files — clean
Remaining (non-blocking) items
-
resolveIdstill runsthis.resolve()for every import once any rule is configured (webpack-loader-compat.ts:268-286). There is no native hookfilterand no cheap extension pre-check before the resolver round-trip. Per AGENTS.md "Filter Vite hooks before JS runs," deriving anincluderegex from the rules'tests (the common extension case) and attaching it asresolveId.filter.idwould keep the common path light. Only bites apps that configure a loader rule, so low urgency. -
Loaders still run once per environment plus once in the config probe; there's no per-
resourcePathoutput cache, andinvokeLoaderSideEffectsruns the chain again at config time. The env-mutation case is idempotent, but non-idempotent loaders (file writes, telemetry, counters) will fire 2-4x. Worth a doc note that loaders must be pure, or a transform cache keyed by resourcePath. -
Edge-case condition semantics (minor, worth a comment or follow-up unit test):
matchesCondition({})returnsfalse; webpack treats an empty condition object as match-all (webpack-loader-compat.ts:73).- A
oneOfparent that also carries its ownusewould emit[child, parent], reversing relative loader order. Webpack rejectsuse+oneOfon the same rule, so this is invalid-config territory, but a unit test pinning intent would be cheap.
None of these block the env-var parity fix that motivates the PR. The core fix is correct and well tested.
|
Review posted to PR #2348. SummaryI re-reviewed PR #2348 at the current head ( Confirmed fixed (with tests):
Local verification (all green): webpack-loader-compat 9/9, image-imports 27/27, next-config webpack 11/11, build-optimization 149/149, Remaining non-blocking items I flagged:
None block the env-var parity fix. The core fix is correct and well tested. |
1 participant
Summary
Validation
vp check packages/vinext/src/plugins/webpack-loader-compat.ts packages/vinext/src/config/next-config.ts packages/vinext/src/index.ts tests/webpack-loader-compat.test.tsvp test run tests/webpack-loader-compat.test.tsvp test run tests/next-config.test.ts -t "webpack loader callbacks|webpack config"ENOSPCbefore deployment and did not reach assertions; exact-head CI will classify end-to-endFixes the baseline failure in
test/e2e/app-dir/webpack-loader-set-environment-variable/webpack-loader-set-environment-variable.test.tsfrom run 28143992598.