fix: keep CJS named exports intact when bundling node_modules in dev - #108
Merged
Conversation
The angular-linker plugin declined every .m?js file that does not require linking when advancedOptimizations is off, i.e. in dev. esbuild then handed those files to @chialab/esbuild-plugin-commonjs, which misdetects a trailing `if (typeof module === 'object')` UMD sniff, wraps the body in an IIFE invoked with `exports === void 0` and emits ESM with only a default export. Named imports then fail to resolve, e.g. `No matching export in node_modules/quill-delta/dist/Delta.js for import "AttributeMap"` when bundling quill via primeng. Production was unaffected only because the linker claims every .m?js there, shadowing the broken plugin. Claiming the file untransformed in dev makes both modes use esbuild's native CommonJS interop, which handles these packages correctly. Refs #83
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
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.
Refs #83. Supersedes #89 (closed) — same branch, much smaller approach.
Problem
ng servefails while bundling shared externals;ng buildsucceeds:11 errors in total, also hitting
quill/blots/scroll.jsandquill/core.jsforOp/OpIterator.Reproduction: https://github.com/leo2823/quill-issue (clean checkout, no changes needed).
Root cause
quill-delta/dist/Delta.jsis plain, statically analysable CJS — esbuild handles it correctlyunaided. The breakage comes from
@chialab/esbuild-plugin-commonjs, which is registeredunconditionally. The file ends in the classic UMD sniff:
That trips the plugin's UMD detector. It wraps the body in an IIFE invoked with
moduleandexportspassed asvoid 0, so everyexports.X = …assignment is lost and the emitted ESM hasonly a default export. Because the plugin converted the file to real ESM, esbuild now
static-checks the named imports and errors.
Confirmed by ablation: strip only that trailing
ifblock and the same plugin emitsexport { __export0 as AttributeMap, __export2 as Op, __export1 as OpIterator, … }.Why production was unaffected
Both
createAngularLinkerPluginandcommonjsPlugin()registeronLoadon/\.m?js$/, andesbuild uses the first hook that returns non-null. With
advancedOptimizations(= !dev) thelinker claims every
.m?jsfile, so the chialab plugin never seesDelta.jsand esbuild's nativeCJS interop applies. In dev the linker early-returned
nullfor anything withoutɵɵngDeclare,handing the file to the broken plugin.
So prod is not correct because it transforms — it is correct because it accidentally shadows a
broken plugin. This inverts the original diagnosis in #83, which assumed dev was missing a
needed CJS→ESM transform. It is not; it is exposing one that corrupts the module.
This is also why #83 could not be fixed in core.
quill-deltais a transitive dep ofquill,quillis inskip, and neither appears in the app'spackage.json, soshareAllneverproduces a
PackageInfofor it — it is an inlined module inside primeng's bundle, with no entrypoint for core's
synthesizeCjsExportsto hook.The fix
Make the dev fast-path claim the file instead of declining it:
if (!needsLinking && !advancedOptimizations) { - return null; + return { contents, loader: 'js' }; }contentsis already read on the line above, so this costs nothing extra — nojsTransformerrun, dev stays fast. It makes dev take the same path prod already takes, which is the dev/prod
consistency #83 asked for.
Also carried on this branch:
@softarc/native-federation^4.3.2→^4.4.0, which brings core'ssynthesizeCjsExportsfor the shared entry point case (the originaldayjsreport, where theCJS package genuinely is shared). The two changes are complementary: core handles shared entry
points, this handles inlined transitive deps.
Verification
Repro —
ng servebuilds clean. Loaded the served app in headless Chrome with a<p-editor>rendered:
.ql-toolbar,.ql-containerand<div class="ql-editor ql-blank" contenteditable="true">all present, zero console errors.
ql-blankis set by Quill's own scroll logic, which goes throughDelta/Op/AttributeMap— so the named bindings resolve at runtime, not just at build time.Regression test —
node-modules-bundler.spec.tsbuilds a fixture CJS module with the UMD tailthrough the real
[angularLinker, commonjsPlugin]pair in dev mode, then dynamically imports theoutput so the assertion is on runtime values. Reverting the one-liner reproduces the exact
production error message.
No behavioural drift on existing dependency trees. Byte-diffed the dev-mode vendor bundles
before/after across
angular-examples/angular/simple(host + 3 remotes):Blast radius on the primeng/quill tree, measured by running chialab's own
maybeMixedModule/maybeCommonjsModule/transformover all.m?jsfiles in the 17 packagesthat contribute to the shared bundles:
wrapDynamicRequire— the only capability esbuild lacks__commonJSThe zero matters:
wrapDynamicRequireonly rewritesif (typeof require ===/!== …)guards. Everyfile matching that pattern in the trees I scanned is Node-only build tooling (
lmdb,sass,yargs,msgpackr,typescript,compiler-cli) that is never browser-bundled. The 265 arequill-delta,eventemitter3,fast-diff,lodash.clonedeep,lodash.isequal,tslib/tslib.jsand
rxjs/dist/cjs/**— all of which prod has been running through native interop all along.Side effect: 104 primeng files were being fully AST-parsed every dev build only for
wrapDynamicRequireto decide it had nothing to do. Dev bundling now skips that, and skips aredundant second file read.
Known behavioural changes
pluginsentry with anonLoadon.m?jsis now shadowed in dev. Prod alreadyshadows it, so this is prod parity, but no example exercises it and the tests cannot see it.
builderOptions.loaderoverrides for.js/.mjsare ignored in dev (forcedloader: 'js').Same parity argument.
broken in dev too — a latent prod bug becoming visible rather than a new fault.
Deliberately out of scope
.cjsfiles. The linker filter/\.m?js$/does not match.cjs, yet.cjsis inresolveExtensions, so.cjsstill reaches the chialab plugin in both dev and prod and thesame UMD misdetection would hit it. That is pre-existing and already dev/prod symmetric — widening
the filter to
/\.[mc]?js$/would change production behaviour on files never tested through thelinker path, which is a different risk class. Worth its own issue.