Skip to content

Commit

Permalink
Merge pull request #9153 from webpack/bugfix/dll-side-effects
Browse files Browse the repository at this point in the history
fix problem with dll and sideEffects
  • Loading branch information
sokra committed May 20, 2019
2 parents 36c7ab7 + a4bbdae commit 5c63e05
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
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
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
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
@@ -0,0 +1 @@
export { B } from "./h1.js";
2 changes: 2 additions & 0 deletions test/configCases/dll-plugin/0-create-dll/h1.js
@@ -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
@@ -0,0 +1 @@
export const A = "A";
1 change: 1 addition & 0 deletions test/configCases/dll-plugin/0-create-dll/hb.js
@@ -0,0 +1 @@
export const B = "B";
9 changes: 8 additions & 1 deletion test/configCases/dll-plugin/0-create-dll/webpack.config.js
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
@@ -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
@@ -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 5c63e05

Please sign in to comment.