Skip to content

Commit d391f14

Browse files
alexander-akaitclaude
authored andcommitted
fix: keep compound and parenthesized CommonJS re-exports valid
A compound assignment to exports combines with the current value, so it is no re-export. A binding read through parentheses drops them with what follows. Cover shadowing, ASI, externals, ESM re-exporters and rebuilds flipping the binding.
1 parent 61cb497 commit d391f14

18 files changed

Lines changed: 260 additions & 15 deletions

File tree

‎lib/dependencies/commonjs/CommonJsExportRequireDependency.js‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -505,9 +505,10 @@ makeSerializable(CommonJsExportRequireDependency, [
505505
* @param {Module | null} importedModule the re-exported module
506506
* @param {ModuleGraph} moduleGraph the module graph
507507
* @param {RuntimeSpec} runtime the runtime
508+
* @param {boolean} used whether the re-export is used, so its value must be read
508509
* @returns {string | undefined} the access to append to the binding, undefined when it can't be read
509510
*/
510-
const getBindingAccess = (dep, importedModule, moduleGraph, runtime) => {
511+
const getBindingAccess = (dep, importedModule, moduleGraph, runtime, used) => {
511512
if (!dep.bindingRange || !dep.bindingDependency || !importedModule) return;
512513
const connection = moduleGraph.getConnection(dep.bindingDependency);
513514
if (
@@ -524,23 +525,23 @@ const getBindingAccess = (dep, importedModule, moduleGraph, runtime) => {
524525
const usedImported = /** @type {string | string[] | false} */ (
525526
moduleGraph.getExportsInfo(importedModule).getUsedName(ids, runtime)
526527
);
527-
if (!usedImported) return;
528-
return propertyAccess(/** @type {string[]} */ (usedImported));
528+
if (usedImported) {
529+
return propertyAccess(/** @type {string[]} */ (usedImported));
530+
}
531+
// an unused re-export of an unused member keeps only the load the binding did
532+
if (!used) return "";
529533
};
530534

531535
/**
532536
* Replaces what follows the binding up to `end` (its member chain, a closing `})`).
537+
* Nothing follows a bare binding, whose access is then empty too.
533538
* @param {ReplaceSource} source the source
534539
* @param {Range} bindingRange range of the binding identifier
535540
* @param {number} end end of the replaced code
536541
* @param {string} content replacement
537542
*/
538543
const replaceAfterBinding = (source, bindingRange, end, content) => {
539-
if (end > bindingRange[1]) {
540-
source.replace(bindingRange[1], end - 1, content);
541-
} else if (content) {
542-
source.insert(bindingRange[1], content);
543-
}
544+
if (end > bindingRange[1]) source.replace(bindingRange[1], end - 1, content);
544545
};
545546

546547
CommonJsExportRequireDependency.Template = class CommonJsExportRequireDependencyTemplate extends (
@@ -593,7 +594,8 @@ CommonJsExportRequireDependency.Template = class CommonJsExportRequireDependency
593594
dep,
594595
importedModule,
595596
moduleGraph,
596-
runtime
597+
runtime,
598+
Boolean(used)
597599
);
598600
if (bindingAccess !== undefined) {
599601
const bindingRange = /** @type {Range} */ (dep.bindingRange);
@@ -621,10 +623,15 @@ CommonJsExportRequireDependency.Template = class CommonJsExportRequireDependency
621623
)}, ${JSON.stringify(used[used.length - 1])}, `
622624
);
623625
}
624-
const valueEnd =
625-
type === "expression"
626-
? dep.range[1]
627-
: /** @type {Range} */ (dep.valueRange)[1];
626+
let valueEnd = dep.range[1];
627+
if (type !== "expression") {
628+
const valueRange = /** @type {Range} */ (dep.valueRange);
629+
// `(ns).a`: the value opens before the binding; drop that with what follows it
630+
if (valueRange[0] < bindingRange[0]) {
631+
source.replace(valueRange[0], bindingRange[0] - 1, "");
632+
}
633+
valueEnd = valueRange[1];
634+
}
628635
replaceAfterBinding(source, bindingRange, valueEnd, bindingAccess);
629636
return;
630637
}

‎lib/dependencies/commonjs/CommonJsExportsParserPlugin.js‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -516,8 +516,11 @@ class CommonJsExportsParserPlugin {
516516
*/
517517
const handleAssignExport = (expr, base, members) => {
518518
if (ESMExports.isEnabled(parser.state)) return;
519-
// Handle reexporting
520-
const requireCall = parseRequireCall(parser, expr.right);
519+
// Handle reexporting; `+=` and the like combine with the current value
520+
const requireCall =
521+
expr.operator === "="
522+
? parseRequireCall(parser, expr.right)
523+
: undefined;
521524
if (
522525
requireCall &&
523526
(members.length === 0 || members[0] !== "__esModule")
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
"use strict";
2+
3+
const ns = require("../targets/cjs?hoisted-shadow");
4+
exports.outer = ns.a;
5+
function withVar() {
6+
exports.beforeVar = ns;
7+
// eslint-disable-next-line no-var
8+
var ns = { local: "var" };
9+
exports.afterVar = ns.local;
10+
}
11+
withVar();
12+
function withFunction() {
13+
exports.functionShadow = typeof ns;
14+
function ns() {}
15+
}
16+
withFunction();
17+
try {
18+
throw { local: "catch" };
19+
} catch (ns) {
20+
exports.catchParam = ns.local;
21+
}
22+
class Holder {
23+
static read() {
24+
return ns.b;
25+
}
26+
}
27+
exports.fromClass = Holder.read();
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
"use strict";
2+
3+
// a compound assignment combines with the current value, so it is no re-export
4+
const ns = require("../targets/cjs?compound");
5+
exports.appended = "x";
6+
exports.appended += ns.a;
7+
exports.direct = "y";
8+
exports.direct += require("../targets/cjs?compound").c;
9+
module.exports.viaModule = "z";
10+
module.exports.viaModule += ns.b;
11+
exports.counter = 1;
12+
exports.counter *= ns.nested.x === "x" ? 3 : 0;
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
"use strict";
2+
3+
const ns = require("../targets/esm-reexporter");
4+
exports.reFoo = ns.reFoo;
5+
exports.starValue = ns.starValue;
6+
exports.reDefault = ns.reDefault;
7+
exports.ns = ns;
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
"use strict";
2+
3+
const external = require("matrix-external");
4+
exports.external = external;
5+
exports.a = external.a;
6+
Object.defineProperty(exports, "b", { enumerable: true, get: () => external.b });
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
"use strict"
2+
3+
const ns = require("../targets/cjs?no-semicolons")
4+
const before = [1, 2]
5+
exports.whole = ns
6+
exports.member = ns.a
7+
exports.unusedWhole = ns
8+
exports.unusedMember = ns.b
9+
;[0].forEach(() => {})
10+
exports.after = before.length
11+
exports.inUnusedExpression = (exports.unusedInner = ns).c
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
"use strict";
2+
3+
const ns = require("../targets/cjs?parenthesized");
4+
exports.wrapped = (ns);
5+
exports.wrappedMember = (ns).a;
6+
exports.wrappedChain = ((ns).nested).x;
7+
exports.innerWrapped = (ns.b);
8+
exports.comment = ns /* between */ .a;
9+
exports.computed = ns["c"];
10+
exports.computedChain = (ns)["nested"]["y"];
11+
Object.defineProperty(exports, "value", { enumerable: true, value: (ns).b });
12+
Object.defineProperty(exports, "valueWhole", { enumerable: true, value: ((ns)) });
13+
Object.defineProperty(exports, "getter", { enumerable: true, get: () => (ns).c });
14+
Object.defineProperty(exports, "getterWhole", { enumerable: true, get: () => ((ns)) });
15+
Object.defineProperty(exports, "method", {
16+
enumerable: true,
17+
get() {
18+
return ((ns)).nested.x;
19+
}
20+
});
21+
exports.info = (ns).usedExports;

‎test/configCases/cjs-tree-shaking/reexport-require-binding-matrix/index.js‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,38 @@ import {
2525
value as usedSefValue,
2626
getter as usedSefGetter
2727
} from "./forms/side-effect-free-used";
28+
import {
29+
wrapped,
30+
wrappedMember,
31+
wrappedChain,
32+
innerWrapped,
33+
comment,
34+
computed,
35+
computedChain,
36+
value as parenthesizedValue,
37+
valueWhole,
38+
getter as parenthesizedGetter,
39+
getterWhole,
40+
method as parenthesizedMethod,
41+
info as parenthesizedInfo
42+
} from "./forms/parenthesized";
43+
import {
44+
whole as noSemicolonsWhole,
45+
member as noSemicolonsMember,
46+
after as noSemicolonsAfter,
47+
inUnusedExpression
48+
} from "./forms/no-semicolons";
49+
import {
50+
reFoo,
51+
starValue,
52+
reDefault,
53+
ns as esmReexporterNs
54+
} from "./forms/esm-reexporter";
55+
import {
56+
external as externalWhole,
57+
a as externalA,
58+
b as externalB
59+
} from "./forms/external";
2860
import { foo as esmFoo, bump as esmBump } from "./forms/esm";
2961
import { name as jsonName, list as jsonList, key as jsonKey } from "./forms/json";
3062
import { text, lazyText } from "./forms/asset";
@@ -200,6 +232,51 @@ it(`[${VARIANT}] re-exports a side-effect-free target read only through re-expor
200232
expect(usedSefGetter).toBe(usedSef);
201233
});
202234

235+
it(`[${VARIANT}] re-exports through parentheses, comments and computed keys`, () => {
236+
expect(wrapped.a).toBe("a");
237+
expect(wrappedMember).toBe("a");
238+
expect(wrappedChain).toBe("x");
239+
expect(innerWrapped).toBe("b");
240+
expect(comment).toBe("a");
241+
expect(computed).toBe("c");
242+
expect(computedChain).toBe("y");
243+
expect(parenthesizedValue).toBe("b");
244+
expect(valueWhole.a).toBe("a");
245+
expect(parenthesizedGetter).toBe("c");
246+
expect(getterWhole.a).toBe("a");
247+
expect(parenthesizedMethod).toBe("x");
248+
shaken(parenthesizedInfo, ["a", "b", "c", "nested", "usedExports"]);
249+
});
250+
251+
it(`[${VARIANT}] re-exports without semicolons, used and unused`, () => {
252+
expect(noSemicolonsWhole.a).toBe("a");
253+
expect(noSemicolonsMember).toBe("a");
254+
expect(noSemicolonsAfter).toBe(2);
255+
expect(inUnusedExpression).toBe("c");
256+
});
257+
258+
it(`[${VARIANT}] re-exports members of an ESM target re-exporting another module`, () => {
259+
expect(reFoo).toBe("foo");
260+
expect(starValue).toBe("star");
261+
expect(reDefault).toBe("default-value");
262+
expect(esmReexporterNs.reFoo).toBe("foo");
263+
expect(esmReexporterNs.__esModule).toBe(true);
264+
});
265+
266+
it(`[${VARIANT}] re-exports an external module`, () => {
267+
expect(externalWhole.a).toBe("ext-a");
268+
expect(externalA).toBe("ext-a");
269+
expect(externalB).toBe("ext-b");
270+
});
271+
272+
it(`[${VARIANT}] keeps compound assignments to exports as written`, () => {
273+
const exports = require("./forms/compound");
274+
expect(exports.appended).toBe("xa");
275+
expect(exports.direct).toBe("yc");
276+
expect(exports.viaModule).toBe("zb");
277+
expect(exports.counter).toBe(3);
278+
});
279+
203280
it(`[${VARIANT}] re-exports an ESM target as its namespace, with live bindings`, () => {
204281
expect(esmFoo).toBe("foo");
205282
const exports = require("./forms/esm");
@@ -267,6 +344,16 @@ it(`[${VARIANT}] leaves a binding of an AMD local module alone`, () => {
267344
expect(exports.localMember).toBe("amd");
268345
});
269346

347+
it(`[${VARIANT}] resolves a binding name shadowed by hoisting, catch and classes`, () => {
348+
const exports = require("./bailouts/hoisted-shadow");
349+
expect(exports.outer).toBe("a");
350+
expect(exports.beforeVar).toBe(undefined);
351+
expect(exports.afterVar).toBe("var");
352+
expect(exports.functionShadow).toBe("function");
353+
expect(exports.catchParam).toBe("catch");
354+
expect(exports.fromClass).toBe("b");
355+
});
356+
270357
it(`[${VARIANT}] treats a destructured require as plain values`, () => {
271358
const exports = require("./bailouts/destructured");
272359
expect(exports.a).toBe("a");
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
export { foo as reFoo } from "./esm";
2+
export * from "./esm-star";
3+
export { default as reDefault } from "./esm";

0 commit comments

Comments
 (0)