Skip to content

Commit

Permalink
fix problem with dll and sideEffects
Browse files Browse the repository at this point in the history
allow to redirect module and id in reexports
SideEffects plugin redirects reexports too
fixes #9146
  • Loading branch information
sokra committed May 20, 2019
1 parent 36c7ab7 commit a4bbdae
Show file tree
Hide file tree
Showing 10 changed files with 77 additions and 28 deletions.
28 changes: 19 additions & 9 deletions lib/dependencies/HarmonyExportImportedSpecifierDependency.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ class HarmonyExportImportedSpecifierDependency extends HarmonyImportDependency {
) {
super(request, originModule, sourceOrder, parserScope);
this.id = id;
this.redirectedId = undefined;
this.name = name;
this.activeExports = activeExports;
this.otherStarExports = otherStarExports;
Expand All @@ -60,9 +61,13 @@ class HarmonyExportImportedSpecifierDependency extends HarmonyImportDependency {
return "harmony export imported specifier";
}

get _id() {
return this.redirectedId || this.id;
}

getMode(ignoreUnused) {
const name = this.name;
const id = this.id;
const id = this._id;
const used = this.originModule.isUsed(name);
const importedModule = this._module;

Expand Down Expand Up @@ -288,7 +293,7 @@ class HarmonyExportImportedSpecifierDependency extends HarmonyImportDependency {
};
}

const importedModule = this.module;
const importedModule = this._module;

if (!importedModule) {
// no imported module available
Expand Down Expand Up @@ -350,11 +355,11 @@ class HarmonyExportImportedSpecifierDependency extends HarmonyImportDependency {
// It's not an harmony module
if (
this.originModule.buildMeta.strictHarmonyModule &&
this.id !== "default"
this._id !== "default"
) {
// In strict harmony modules we only support the default export
const exportName = this.id
? `the named export '${this.id}'`
const exportName = this._id
? `the named export '${this._id}'`
: "the namespace object";
return [
new HarmonyLinkingError(
Expand All @@ -365,20 +370,20 @@ class HarmonyExportImportedSpecifierDependency extends HarmonyImportDependency {
return;
}

if (!this.id) {
if (!this._id) {
return;
}

if (importedModule.isProvided(this.id) !== false) {
if (importedModule.isProvided(this._id) !== false) {
// It's provided or we are not sure
return;
}

// We are sure that it's not provided
const idIsNotNameMessage =
this.id !== this.name ? ` (reexported as '${this.name}')` : "";
this._id !== this.name ? ` (reexported as '${this.name}')` : "";
const errorMessage = `"export '${
this.id
this._id
}'${idIsNotNameMessage} was not found in '${this.userRequest}'`;
return [new HarmonyLinkingError(errorMessage)];
}
Expand All @@ -402,6 +407,11 @@ class HarmonyExportImportedSpecifierDependency extends HarmonyImportDependency {
importedModule.used + stringifiedUsedExport + stringifiedProvidedExport
);
}

disconnect() {
super.disconnect();
this.redirectedId = undefined;
}
}

module.exports = HarmonyExportImportedSpecifierDependency;
Expand Down
5 changes: 3 additions & 2 deletions lib/optimize/SideEffectsFlagPlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,9 @@ class SideEffectsFlagPlugin {
const reason = module.reasons[i];
const dep = reason.dependency;
if (
dep instanceof HarmonyImportSpecifierDependency &&
!dep.namespaceObjectAsContext
dep instanceof HarmonyExportImportedSpecifierDependency ||
(dep instanceof HarmonyImportSpecifierDependency &&
!dep.namespaceObjectAsContext)
) {
const mapping = map.get(dep.id);
if (mapping) {
Expand Down
8 changes: 4 additions & 4 deletions test/__snapshots__/StatsTestCases.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -2231,16 +2231,14 @@ Entrypoint main = main.js
[0] ./components/src/CompAB/index.js 87 bytes [built]
[no exports used]
harmony side effect evaluation ./CompAB [3] ./components/src/index.js 1:0-40
harmony export imported specifier ./CompAB [3] ./components/src/index.js 1:0-40
harmony export imported specifier ./CompAB [3] ./components/src/index.js 1:0-40
[1] ./components/src/CompC/CompC.js 33 bytes [built]
[no exports used]
harmony side effect evaluation ./CompC [2] ./components/src/CompC/index.js 1:0-34
harmony export imported specifier ./CompC [2] ./components/src/CompC/index.js 1:0-34
harmony export imported specifier ./CompC [3] ./components/src/index.js 2:0-43 (skipped side-effect-free modules)
[2] ./components/src/CompC/index.js 34 bytes [built]
[no exports used]
harmony side effect evaluation ./CompC [3] ./components/src/index.js 2:0-43
harmony export imported specifier ./CompC [3] ./components/src/index.js 2:0-43
[3] ./components/src/index.js 84 bytes [built]
[no exports used]
harmony side effect evaluation ./components [6] ./foo.js 1:0-37
Expand All @@ -2249,6 +2247,7 @@ Entrypoint main = main.js
[only some exports used: default]
harmony side effect evaluation ./CompA [0] ./components/src/CompAB/index.js 1:0-43
harmony export imported specifier ./CompA [0] ./components/src/CompAB/index.js 1:0-43
harmony export imported specifier ./CompAB [3] ./components/src/index.js 1:0-40 (skipped side-effect-free modules)
harmony import specifier ./components [6] ./foo.js 3:20-25 (skipped side-effect-free modules)
harmony import specifier ./components ./main.js 3:15-20 (skipped side-effect-free modules)
[5] ./components/src/CompAB/utils.js 97 bytes {0} [built]
Expand All @@ -2266,11 +2265,12 @@ Entrypoint main = main.js
| [only some exports used: default]
| harmony side effect evaluation ./CompB [0] ./components/src/CompAB/index.js 2:0-43
| harmony export imported specifier ./CompB [0] ./components/src/CompAB/index.js 2:0-43
| harmony export imported specifier ./CompAB [3] ./components/src/index.js 1:0-40 (skipped side-effect-free modules)
| harmony import specifier ./components ./main.js 4:15-20 (skipped side-effect-free modules)"
`;

exports[`StatsTestCases should print correct stats for side-effects-simple-unused 1`] = `
"Hash: 518ad50555bd6e64c28b
"Hash: a5866e21e8cfa3ae1a89
Time: Xms
Built at: Thu Jan 01 1970 00:00:00 GMT
Asset Size Chunks Chunk Names
Expand Down
1 change: 1 addition & 0 deletions test/configCases/dll-plugin/0-create-dll/h.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { B } from "./h1.js";
2 changes: 2 additions & 0 deletions test/configCases/dll-plugin/0-create-dll/h1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { A } from "./ha.js";
export { B } from "./hb.js";
1 change: 1 addition & 0 deletions test/configCases/dll-plugin/0-create-dll/ha.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const A = "A";
1 change: 1 addition & 0 deletions test/configCases/dll-plugin/0-create-dll/hb.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const B = "B";
9 changes: 8 additions & 1 deletion test/configCases/dll-plugin/0-create-dll/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ var path = require("path");
var webpack = require("../../../../");

module.exports = {
entry: ["./a", "./b", "./_d", "./_e", "./f", "./g.abc"],
entry: ["./a", "./b", "./_d", "./_e", "./f", "./g.abc", "./h"],
resolve: {
extensions: [".js", ".jsx"]
},
Expand All @@ -19,9 +19,16 @@ module.exports = {
options: {
test: 1
}
},
{
test: /0-create-dll.h/,
sideEffects: false
}
]
},
optimization: {
sideEffects: true
},
plugins: [
new webpack.DllPlugin({
path: path.resolve(
Expand Down
24 changes: 17 additions & 7 deletions test/configCases/dll-plugin/1-use-dll/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import d from "dll/d";
import { x1, y2 } from "./e";
import { x2, y1 } from "dll/e";
import { B } from "dll/h";

it("should load a module from dll", function() {
expect(require("dll/a")).toBe("a");
Expand All @@ -11,10 +12,12 @@ it("should load a module of non-default type without extension from dll", functi
});

it("should load an async module from dll", function(done) {
require("dll/b")().then(function(c) {
expect(c).toEqual(nsObj({ default: "c" }));
done();
}).catch(done);
require("dll/b")()
.then(function(c) {
expect(c).toEqual(nsObj({ default: "c" }));
done();
})
.catch(done);
});

it("should load an harmony module from dll (default export)", function() {
Expand All @@ -33,7 +36,9 @@ it("should load a module with loader applied", function() {
});

it("should give modules the correct ids", function() {
expect(Object.keys(__webpack_modules__).filter(m => !m.startsWith("../.."))).toEqual([
expect(
Object.keys(__webpack_modules__).filter(m => !m.startsWith("../.."))
).toEqual([
"./index.js",
"dll-reference ../0-create-dll/dll.js",
"dll/a.js",
Expand All @@ -43,6 +48,11 @@ it("should give modules the correct ids", function() {
"dll/e1.js",
"dll/e2.js",
"dll/f.jsx",
"dll/g.abc.js"
]);
"dll/g.abc.js",
"dll/h.js"
]);
});

it("should not crash on side-effect-free modules", function() {
expect(B).toBe("B");
});
26 changes: 21 additions & 5 deletions test/configCases/dll-plugin/2-use-dll-without-scope/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import d from "../0-create-dll/d";
import { x1, y2 } from "./e";
import { x2, y1 } from "../0-create-dll/e";
import { B } from "../0-create-dll/h";
import { A } from "../0-create-dll/h1";

it("should load a module from dll", function() {
expect(require("../0-create-dll/a")).toBe("a");
Expand All @@ -11,10 +13,12 @@ it("should load a module of non-default type without extension from dll", functi
});

it("should load an async module from dll", function(done) {
require("../0-create-dll/b")().then(function(c) {
expect(c).toEqual(nsObj({ default: "c" }));
done();
}).catch(done);
require("../0-create-dll/b")()
.then(function(c) {
expect(c).toEqual(nsObj({ default: "c" }));
done();
})
.catch(done);
});

it("should load an harmony module from dll (default export)", function() {
Expand All @@ -33,7 +37,9 @@ it("should load a module with loader applied", function() {
});

it("should give modules the correct ids", function() {
expect(Object.keys(__webpack_modules__).filter(m => !m.startsWith("../.."))).toEqual([
expect(
Object.keys(__webpack_modules__).filter(m => !m.startsWith("../.."))
).toEqual([
"../0-create-dll/a.js",
"../0-create-dll/b.js",
"../0-create-dll/d.js",
Expand All @@ -42,7 +48,17 @@ it("should give modules the correct ids", function() {
"../0-create-dll/e2.js",
"../0-create-dll/f.jsx",
"../0-create-dll/g.abc.js",
"../0-create-dll/h.js",
"../0-create-dll/hb.js",
"./index.js",
"dll-reference ../0-create-dll/dll.js"
]);
});

it("should not crash on side-effect-free modules", function() {
expect(B).toBe("B");
});

it("should be able to reference side-effect-free reexport-only module", function() {
expect(A).toBe("A");
});

0 comments on commit a4bbdae

Please sign in to comment.