Fix tree-shaking wildcards with sideEffects: false #1682
Conversation
fathyb
force-pushed
the
fix/tree-shake-side-effects
branch
from
33377d0
to
6cb093d
Compare
src/packagers/JSConcatPackager.js
Outdated
| @@ -78,6 +78,25 @@ class JSConcatPackager extends Packager { | |||
| return this.size; | |||
| } | |||
|
|
|||
| findExportAsset(asset, name) { | |||
There was a problem hiding this comment.
might be a good idea to share some of this function with the similar one in concat.js. probably should ensure that they work the same way.
fathyb
force-pushed
the
fix/tree-shake-side-effects
branch
from
6cb093d
to
925b236
Compare
| if (name === '*') { | ||
| this.markUsedExports(dep); | ||
| } | ||
|
|
||
| this.markUsed(dep, name); |
There was a problem hiding this comment.
should this be in an else? Otherwise we pass * as the name to markUsed
There was a problem hiding this comment.
a.js:
import * as b from './b'
console.log(b)b.js: <==b.jsshould bedephere, if we removemarkUsedit'll get trimmed because it doesn't have any exports/import in thecacheData(just 2 elements inwildcards)
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
src/packagers/JSConcatPackager.js
Outdated
| let {id} = this.findExportModule(mod.id, name); | ||
| mod = this.assets.get(id) || mod; | ||
|
|
||
| let exp = mod.cacheData.exports[name]; | ||
| if (Array.isArray(exp)) { |
There was a problem hiding this comment.
this should always be false now right, since you already followed the exports through in findExportModule?
There was a problem hiding this comment.
Not in a code-splitted packager, doesn't really make sense to fallback on mod though. Replaced it with a return fallback.
src/packagers/JSConcatPackager.js
Outdated
| 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ah, it's a piece of code I added early but forgot to remove
Fixes #1553