Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tree-shaking wildcards with sideEffects: false #1682

Merged
merged 3 commits into from Jul 9, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
72 changes: 69 additions & 3 deletions src/packagers/JSConcatPackager.js
Expand Up @@ -19,6 +19,7 @@ const helpers =
class JSConcatPackager extends Packager {
async start() {
this.addedAssets = new Set();
this.assets = new Map();
this.exposedModules = new Set();
this.externalModules = new Set();
this.size = 0;
Expand All @@ -43,6 +44,8 @@ class JSConcatPackager extends Packager {
this.needsPrelude = true;
}

this.assets.set(asset.id, asset);

for (let mod of asset.depAssets.values()) {
if (
!this.bundle.assets.has(mod) &&
Expand Down Expand Up @@ -88,19 +91,34 @@ class JSConcatPackager extends Packager {
for (let identifier in asset.cacheData.imports) {
let [source, name] = asset.cacheData.imports[identifier];
let dep = asset.depAssets.get(asset.dependencies.get(source));

if (name === '*') {
this.markUsedExports(dep);
}

this.markUsed(dep, name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be in an else? Otherwise we pass * as the name to markUsed

Copy link
Contributor Author

@fathyb fathyb Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • a.js :
import * as b from './b'

console.log(b)
  • b.js : <== b.js should be dep here, if we remove markUsed it'll get trimmed because it doesn't have any exports/import in the cacheData (just 2 elements in wildcards)
export * from './c'
export * from './d'

Using * seemed like a nice fix to me for this, as it'll keep usedImports.size > 0 and never conflict with existing import. I'm just getting familiar with this part though, so not really sure

}

for (let source of asset.cacheData.wildcards) {
let dep = asset.depAssets.get(asset.dependencies.get(source));
for (let exportIdentifier in dep.cacheData.exports) {
this.markUsed(dep, exportIdentifier);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? Do we really want to mark every export in the wildcard as used? Shouldn't it only be the one matching name? And will markUsed already handle that for us since it is looking up wildcards via findExportModule?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it's a piece of code I added early but forgot to remove

}
}
}

markUsed(mod, id) {
let exp = mod.cacheData.exports[id];
markUsed(mod, name) {
let {id} = this.findExportModule(mod.id, name);
mod = this.assets.get(id) || mod;

let exp = mod.cacheData.exports[name];
if (Array.isArray(exp)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should always be false now right, since you already followed the exports through in findExportModule?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in a code-splitted packager, doesn't really make sense to fallback on mod though. Replaced it with a return fallback.

let depMod = mod.depAssets.get(mod.dependencies.get(exp[0]));
return this.markUsed(depMod, exp[1]);
}

this.markUsedExports(mod);
mod.usedExports.add(id);
mod.usedExports.add(name);
}

getExportIdentifier(asset) {
Expand Down Expand Up @@ -521,6 +539,54 @@ class JSConcatPackager extends Packager {

await super.write(output);
}

resolveModule(id, name) {
let module = this.assets.get(+id);
return module.depAssets.get(module.dependencies.get(name));
}

findExportModule(id, name, replacements) {
let asset = this.assets.get(+id);
let exp =
asset &&
Object.prototype.hasOwnProperty.call(asset.cacheData.exports, name)
? asset.cacheData.exports[name]
: null;

// If this is a re-export, find the original module.
if (Array.isArray(exp)) {
let mod = this.resolveModule(id, exp[0]);
return this.findExportModule(mod.id, exp[1], replacements);
}

// If this module exports wildcards, resolve the original module.
// Default exports are excluded from wildcard exports.
let wildcards = asset && asset.cacheData.wildcards;
if (wildcards && name !== 'default' && name !== '*') {
for (let source of wildcards) {
let mod = this.resolveModule(id, source);
let m = this.findExportModule(mod.id, name, replacements);
if (m.identifier) {
return m;
}
}
}

// If this is a wildcard import, resolve to the exports object.
if (asset && name === '*') {
exp = `$${id}$exports`;
}

if (replacements && replacements.has(exp)) {
exp = replacements.get(exp);
}

return {
identifier: exp,
name,
id
};
}
}

module.exports = JSConcatPackager;
87 changes: 23 additions & 64 deletions src/scope-hoisting/concat.js
Expand Up @@ -15,75 +15,26 @@ const THROW_TEMPLATE = template('$parcel$missingModule(MODULE)');
const REQUIRE_TEMPLATE = template('require(ID)');

module.exports = (packager, ast) => {
let {addedAssets} = packager;
let {assets} = packager;
let replacements = new Map();
let imports = new Map();
let referenced = new Set();

let assets = Array.from(addedAssets).reduce((acc, asset) => {
acc[asset.id] = asset;

return acc;
}, {});

// Build a mapping of all imported identifiers to replace.
for (let asset of addedAssets) {
for (let asset of assets.values()) {
for (let name in asset.cacheData.imports) {
let imp = asset.cacheData.imports[name];
imports.set(name, [resolveModule(asset.id, imp[0]), imp[1]]);
imports.set(name, [packager.resolveModule(asset.id, imp[0]), imp[1]]);
}
}

function resolveModule(id, name) {
let module = assets[id];
return module.depAssets.get(module.dependencies.get(name));
}

function findExportModule(id, name) {
let module = assets[id];
let exp =
module &&
Object.prototype.hasOwnProperty.call(module.cacheData.exports, name)
? module.cacheData.exports[name]
: null;

// If this is a re-export, find the original module.
if (Array.isArray(exp)) {
let mod = resolveModule(id, exp[0]);
return findExportModule(mod.id, exp[1]);
}

// If this module exports wildcards, resolve the original module.
// Default exports are excluded from wildcard exports.
let wildcards = module && module.cacheData.wildcards;
if (wildcards && name !== 'default' && name !== '*') {
for (let source of wildcards) {
let m = findExportModule(resolveModule(id, source).id, name);
if (m.identifier) {
return m;
}
}
}

// If this is a wildcard import, resolve to the exports object.
if (module && name === '*') {
exp = `$${id}$exports`;
}

if (replacements.has(exp)) {
exp = replacements.get(exp);
}

return {
identifier: exp,
name,
id
};
}

function replaceExportNode(module, originalName, path) {
let {identifier, name, id} = findExportModule(module.id, originalName);
let mod = assets[id];
let {identifier, name, id} = packager.findExportModule(
module.id,
originalName,
replacements
);
let mod = assets.get(id);
let node;

if (identifier) {
Expand Down Expand Up @@ -190,10 +141,10 @@ module.exports = (packager, ast) => {
);
}

let mod = resolveModule(id.value, source.value);
let mod = packager.resolveModule(id.value, source.value);

if (!mod) {
if (assets[id.value].dependencies.get(source.value).optional) {
if (assets.get(id.value).dependencies.get(source.value).optional) {
path.replaceWith(
THROW_TEMPLATE({MODULE: t.stringLiteral(source.value)})
);
Expand All @@ -204,7 +155,7 @@ module.exports = (packager, ast) => {
}
} else {
let node;
if (assets[mod.id]) {
if (assets.get(mod.id)) {
// Replace with nothing if the require call's result is not used.
if (!isUnusedValue(path)) {
let name = `$${mod.id}$exports`;
Expand Down Expand Up @@ -241,7 +192,7 @@ module.exports = (packager, ast) => {
);
}

let mapped = assets[id.value];
let mapped = assets.get(id.value);
let dep = mapped.dependencies.get(source.value);
let mod = mapped.depAssets.get(dep);
let bundles = mod.id;
Expand Down Expand Up @@ -286,7 +237,11 @@ module.exports = (packager, ast) => {
continue;
}

let {identifier} = findExportModule(match[1], key.name, path);
let {identifier} = packager.findExportModule(
match[1],
key.name,
replacements
);
if (identifier) {
replace(value.name, identifier, p);
}
Expand Down Expand Up @@ -336,7 +291,11 @@ module.exports = (packager, ast) => {
// If it's a $id$exports.name expression.
if (match) {
let name = t.isIdentifier(property) ? property.name : property.value;
let {identifier} = findExportModule(match[1], name, path);
let {identifier} = packager.findExportModule(
match[1],
name,
replacements
);

// Check if $id$export$name exists and if so, replace the node by it.
if (identifier) {
Expand Down
@@ -0,0 +1,3 @@
import {foo} from 'bar'

output = foo

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions test/scope-hoisting.js
Expand Up @@ -271,6 +271,16 @@ describe('scope hoisting', function() {
assert.deepEqual(output, 4);
});

it('supports wildcards with sideEffects: false', async function() {
let b = await bundle(
__dirname +
'/integration/scope-hoisting/es6/side-effects-false-wildcards/a.js'
);
let output = await run(b);

assert.deepEqual(output, 'bar');
});

it('missing exports should be replaced with an empty object', async function() {
let b = await bundle(
__dirname + '/integration/scope-hoisting/es6/empty-module/a.js'
Expand Down