[2.x] fix: anchor webpack-config extension source detection to the real src directory - #4770
[2.x] fix: anchor webpack-config extension source detection to the real src directory#4770PrimateCoder wants to merge 2 commits into
Conversation
PrimateCoder
changed the title
imorland
left a comment
imorland
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis is right. Some of what I checked, for the record:
- The chunk-name symptom is actually worse than you describe.
absolutePathToImport.split('src/')[1]returns the checkout path fragment, which doesn't vary by module — so every dynamic import in such a checkout gets the samewebpackChunkName(acme/js/in your fixture). WithwebpackMode: 'lazy-once'that collapses all lazy chunks into one, rather than just producing an ugly name. Worth saying in the description. - Your tests do pin the fix: reverting only
src/fails three of the four, and they pass after. - The premise holds — webpack compiles a string condition to
str.startsWith(condition)inRuleSetCompiler, so the anchored include has the semantics you're relying on. - I built core's JS before and after: byte-identical across all 44 output files, so the no-op claim checks out.
- Nothing else in the package does loose matching;
autoExportLoaderwas already anchored viathis.rootContext.
One thing you've fixed without claiming it: the urlPath rewrite in RegisterAsyncChunksPlugin replaces a greedy .*/src/ (last match) with a slice from the project's own src (first). For a nested src — say <proj>/src/forum/src/Foo.ts — the old code produced Foo and the new produces forum/src/Foo, which is what autoExportLoader registers. So it also removes a real disagreement between the two. Worth calling out.
Three things before I approve:
1. Windows confirmation. include: /src/ was a separator-agnostic substring regex; the replacement is an exact, case-sensitive prefix string. On Windows, drive-letter and path casing can differ between process.cwd() and webpack's module.resource, and a mismatch here fails silently — the loaders just don't run, so you get a successful build with no flarum.reg.add calls rather than an error. This code supports Windows deliberately (path.sep == '\\' branches in two places), so I'd like a build confirmed on Windows before this goes in.
2. Unify on webpack's context. There are now four bases for the same idea: index.cjs and RegisterAsyncChunksPlugin use process.cwd(), autoChunkNameLoader uses this.rootContext || process.cwd(), and autoExportLoader uses this.rootContext. They're identical today only because nothing sets context and webpack defaults it to cwd. Since the whole point of this PR is to stop inferring the source root, I'd rather it came from one place:
RegisterAsyncChunksPlugingetscompilerinapply(), so usecompiler.options.context.- Set
context: process.cwd()explicitly inindex.cjsand derive the include from it, so the contract is stated rather than implied. - Drop the
|| process.cwd()fallback inautoChunkNameLoaderto matchautoExportLoader— real webpack always setsrootContext, and your test already passes it.
3. Rebase on current 2.x. The branch is based on rc.4-era 2.x and the checks last ran in June. Nothing has touched these files since, so it should rebase clean — I just want CI to have run against current 2.x before it goes in.
One optional note while you're in there: the include test asserts rule.include equals a value recomputed the same way as the implementation, and the second test hand-rolls its own matches() helper. I checked webpack's condition semantics myself so the fix is right, but neither test would catch it being wrong. tests/compiler.js already runs real webpack compiles — building the config through index.cjs there would exercise the rule properly. Not blocking.
I'll add the changelog entry ((webpack) scope) when this lands, so don't worry about that.
|
I didn't think about Windows, thanks for the call out. Fortunately the testing looks good. Here are the results from orig vs patched. Orig: Patched: |
efc469b to
e6b1423
Compare
|
Thanks for the thorough review! All three blocking items are done: 1. Windows confirmation ✅ Built core's JS natively on Windows (PowerShell) from a checkout under a
2. Unified on webpack's
No behaviour change — the values are identical today; the contract is just stated in one place now. 3. Rebased on current I've also updated the PR description per your notes: the chunk-name symptom is worse than originally stated (the constant name collapses all lazy chunks into one under |
… dir
The config identifies an extension's own source files (under its `src/`
dir) using loose "src" matching: `include: /src/` in index.cjs,
`resource.split(path.sep).includes('src')` / `resource.includes('/src/')`
in RegisterAsyncChunksPlugin, and `absolutePathToImport.includes('src')`
in autoChunkNameLoader.
When a project is checked out under a path that itself contains a "src"
segment (e.g. ~/src/my-ext), node_modules paths also contain "src", so:
the auto-export loader runs on third-party modules (e.g. @babel/runtime
helpers) and emits invalid flarum.reg.add(...) (build fails with "Module
parse failed"); RegisterAsyncChunksPlugin treats node_modules as
extension source and throws "Cannot read properties of undefined
(reading 'includes')"; and autoChunkNameLoader produces chunk names that
leak the checkout path.
Anchor all of these to the extension's actual src directory
(path.resolve(process.cwd(), 'src') / this.rootContext) using directory
prefix matching, and harden the _source._value access. No-op for
checkouts not under a "src" path. Adds regression tests.
Derive the extension's src directory from a single source of truth instead of four separate bases: set `context` explicitly in index.cjs and derive the loader includes from it, use `compiler.options.context` in RegisterAsyncChunksPlugin, and drop the `process.cwd()` fallback in autoChunkNameLoader so it matches autoExportLoader. No behaviour change; the values are identical today since nothing overrides context.
e6b1423 to
857ef13
Compare
2 participants
Fixes a build failure in
js-packages/webpack-configthat occurs when an extension (or core) is checked out under a filesystem path that itself contains asrcsegment (e.g.~/src/my-ext).Problem
The webpack config identifies an extension's own source files (under its
src/dir) using loose "src" matching in several places:index.cjs: loader rules useinclude: /src/— a regex matching the substring "src" anywhere in a module's absolute path.RegisterAsyncChunksPlugin.cjs:module.resource.split(path.sep).includes('src')andmodule.resource.includes('/src/').autoChunkNameLoader.cjs:absolutePathToImport.includes('src')then.split('src/')[1].When the project lives under a path containing a
srcsegment, the absolute paths ofnode_modulesfiles also contain "src", so:@babel/runtime/helpers/esm/defineProperty.js, which containsexport { _defineProperty as default }) and emits invalidflarum.reg.add('…', { _defineProperty as default: … }), failing the build withModule parse failed: Unexpected token.RegisterAsyncChunksPlugintreatsnode_modulesmodules as extension source and throwsTypeError: Cannot read properties of undefined (reading 'includes')when such a module's_source._valueis undefined.autoChunkNameLoadersplits on the firstsrc/in the path, so an extension with dynamic imports gets chunk names that leak the checkout path (e.g.acme/js/…instead offorum/Lazy).This doesn't reproduce in normal CI checkouts because their paths don't contain a
srcsegment — which is why it's easy to miss.Fix
Anchor every "is this file part of the extension's
src?" check to the extension's actualsrcdirectory (path.resolve(process.cwd(), 'src'), or the loader'sthis.rootContext) and match by directory prefix instead of loose substring/segment matching. Also hardens a_source._value?.access with optional chaining.This is a no-op for checkouts not under a
srcpath (where/src/only ever matched the project's ownsrc).process.cwd()is already the basis for entry points, the output dir, andcomposer.json/package.jsonresolution throughout this config, so this introduces no new assumption.Tests
Adds
tests/srcPathAnchoring.test.js(self-contained; does not depend on the monorepocomposer.json):<cwd>/srcinclude rather than a/src/regex, and do not matchnode_modulesunder an unrelated…/src/…path;autoChunkNameLoaderproduces chunk names relative to the extension'ssrceven when the checkout path contains asrcsegment;ext:/flarum/) imports still convert toflarum.reg.asyncModuleImport(...).These fail against the pre-fix code and pass after it.
Verification
Built a real Flarum 2.x extension whose checkout path contains a
srcsegment: the build previously failed with both errors above and now succeeds. A dynamic import yieldsaddChunkModule('…','…','<ns>','forum/Lazy')(correctlysrc-relative) with no checkout-path leakage, and the produced bundle is otherwise byte-identical.Windows: core's JS was also built natively on Windows (PowerShell) from a checkout under a
srcsegment (C:\Users\…\GitHub\src\framework), comparing pre-fix and post-fix clones side by side. Pre-fix, the build fails withModule parse failedon the@babel/runtimehelpers underjs-packages/webpack-config/node_modules(the auto-export loader ran on them). Post-fix, it builds cleanly with 455flarum.reg.addcalls indist/forum.js— the non-zero count rules out a silent prefix mismatch between webpack'scontextandmodule.resourceon Windows.Addendum (from review)
Two corrections/expansions based on review feedback:
split('src/')[1]returns a fragment of the checkout path that is identical for every module in the checkout, so withwebpackMode: 'lazy-once'all dynamic imports receive the samewebpackChunkNameand collapse into a single lazy chunk — not just an ugly name.urlPathrewrite inRegisterAsyncChunksPluginreplaced a greedy.*/src/match (last occurrence) with a slice from the project's ownsrc(first occurrence). For a nestedsrc(e.g.<proj>/src/forum/src/Foo.ts) the old code producedFoowhileautoExportLoaderregisteredforum/src/Foo; both now agree onforum/src/Foo.The source root is now unified on webpack's
context(set explicitly inindex.cjs, derived inRegisterAsyncChunksPluginviacompiler.options.context, andthis.rootContextin the loaders) rather than four separateprocess.cwd()-based bases.