From 6255900af23188713485246b029b49f7132e69e0 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Thu, 30 Aug 2018 08:17:53 -0400 Subject: [PATCH 1/3] Breaking test for #1068 --- test/tree_shaking_test.js | 8 ++++++++ test/treeshake/basics/main.js | 9 ++++++--- .../basics/node_modules/dep5/another-es-module.js | 8 ++++++++ test/treeshake/basics/node_modules/dep5/es-module.js | 5 +++++ test/treeshake/basics/node_modules/dep5/main.js | 2 ++ 5 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 test/treeshake/basics/node_modules/dep5/another-es-module.js create mode 100644 test/treeshake/basics/node_modules/dep5/es-module.js diff --git a/test/tree_shaking_test.js b/test/tree_shaking_test.js index 400e74d4..e22e1dd3 100644 --- a/test/tree_shaking_test.js +++ b/test/tree_shaking_test.js @@ -126,6 +126,14 @@ describe("Tree-shaking", function(){ assert.equal(connect.didFail, true, "marked as failed"); }); }); + + describe.only("ES modules imported by CommonJS modules", function() { + it("Tree-shakes their ES module dependencies", function() { + let mod = app.dep5dep5AnotherESModule; + assert.equal(typeof mod.mainThing, "function", "Kept the used export"); + assert.equal(typeof mod.another, "undefined", "Removed the unused export"); + }); + }); }); describe("treeShaking: false", function(){ diff --git a/test/treeshake/basics/main.js b/test/treeshake/basics/main.js index f9ae775e..b4c103b1 100644 --- a/test/treeshake/basics/main.js +++ b/test/treeshake/basics/main.js @@ -53,7 +53,8 @@ export default function(){ steal.import("dep4/other"), steal.import("dep4/and-another"), steal.import("~/from-exports"), - shouldFail + shouldFail, + steal.import("dep5/another-es-module") ]); return p @@ -64,7 +65,8 @@ export default function(){ dep4Other, dep4AndAnother, fromExports, - canConnect + canConnect, + dep5dep5AnotherESModule ]) => { return { anon, @@ -75,7 +77,8 @@ export default function(){ dep4AndAnother, fromExports, dep5, - canConnect + canConnect, + dep5dep5AnotherESModule }; }); }; diff --git a/test/treeshake/basics/node_modules/dep5/another-es-module.js b/test/treeshake/basics/node_modules/dep5/another-es-module.js new file mode 100644 index 00000000..de339c71 --- /dev/null +++ b/test/treeshake/basics/node_modules/dep5/another-es-module.js @@ -0,0 +1,8 @@ +export function mainThing() { + +} + +// This isn't used. This should be tree shaken. +export function another() { + +} diff --git a/test/treeshake/basics/node_modules/dep5/es-module.js b/test/treeshake/basics/node_modules/dep5/es-module.js new file mode 100644 index 00000000..6eb7e519 --- /dev/null +++ b/test/treeshake/basics/node_modules/dep5/es-module.js @@ -0,0 +1,5 @@ +import { another, mainThing } from "./another-es-module"; + +window.mainThing = mainThing; + +// Notice that I do not use `another` diff --git a/test/treeshake/basics/node_modules/dep5/main.js b/test/treeshake/basics/node_modules/dep5/main.js index d0b7285f..6a4a4858 100644 --- a/test/treeshake/basics/node_modules/dep5/main.js +++ b/test/treeshake/basics/node_modules/dep5/main.js @@ -1,3 +1,5 @@ +require("./es-module"); + exports.doStuff = function(){ return "worked"; }; From 59ced05637538eaf4ecbf5bd5e94e6cff4f67c05 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Thu, 30 Aug 2018 08:33:57 -0400 Subject: [PATCH 2/3] Enable traversing non-ES modules imports This makes it so that if an ES module is only imported by non-ES modules, it is still included in the tree-shaking algorithm and can be tree-shaken. Fixes #1068 --- lib/graph/treeshake.js | 19 +++++++++++++------ test/tree_shaking_test.js | 2 +- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/graph/treeshake.js b/lib/graph/treeshake.js index f75260a9..6f03d9ae 100644 --- a/lib/graph/treeshake.js +++ b/lib/graph/treeshake.js @@ -112,9 +112,17 @@ function loadFromGraph(getNode) { } } + let code = ''; + + if(node) { + // Add deps as regular imports + for(let specifier of node.load.metadata.deps) { + code += `import "${specifier}";\n`; + } + } + // Expose named exports so that dependant modules will tree-shake properly. if(needToExport.size) { - let code = ''; for(let exp of needToExport) { if(exp === "default") { code += "export default {};\n"; @@ -122,19 +130,18 @@ function loadFromGraph(getNode) { code += `export let ${exp} = {};\n`; } } - - return code; } else { // Prevent tree shaking modules that are side-effectual like CSS if(sideEffectualModule(node)) { - return ` + code += ` export function one() {window.ONE = {}}; one(); `.trim(); + } else { + code += "export default {}"; } - - return "export default {}"; } + return code; } return source.node(node); diff --git a/test/tree_shaking_test.js b/test/tree_shaking_test.js index e22e1dd3..c369a382 100644 --- a/test/tree_shaking_test.js +++ b/test/tree_shaking_test.js @@ -127,7 +127,7 @@ describe("Tree-shaking", function(){ }); }); - describe.only("ES modules imported by CommonJS modules", function() { + describe("ES modules imported by CommonJS modules", function() { it("Tree-shakes their ES module dependencies", function() { let mod = app.dep5dep5AnotherESModule; assert.equal(typeof mod.mainThing, "function", "Kept the used export"); From aa98380c2d3c336c20e6957f3eb8c31fb6b00ec3 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Thu, 30 Aug 2018 09:36:32 -0400 Subject: [PATCH 3/3] Import and use ES modules' namespace object. --- lib/graph/treeshake.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/graph/treeshake.js b/lib/graph/treeshake.js index 6f03d9ae..09cec20c 100644 --- a/lib/graph/treeshake.js +++ b/lib/graph/treeshake.js @@ -116,8 +116,10 @@ function loadFromGraph(getNode) { if(node) { // Add deps as regular imports - for(let specifier of node.load.metadata.deps) { - code += `import "${specifier}";\n`; + for(let i = 0; i < node.load.metadata.deps.length; i++) { + let specifier = node.load.metadata.deps[i]; + code += `import * as dep${i} from "${specifier}";\n`; + code += `global.someFunction(dep${i}.default);\n`; } }