-
-
Notifications
You must be signed in to change notification settings - Fork 9.1k
Description
Do you want to request a feature or report a bug?
Bug
What is the current behavior?
Webpack 3/next with partial scope hoisting will rename class variables if there would be a conflict between two concatenated modules.
However it currently does not rename references to classes inside class methods, causing either undefined references (because there is no longer a class with that name in the scope anymore), or incorrect behaviour (accessing another class that also happens to have that name).
If the current behavior is a bug, please provide the steps to reproduce.
Here's a repo that reproduces the bug:
https://github.com/willheslam/webpack-renaming-bug
- yarn install
- yarn build
- yarn test
What is currently printed to the console:
Expecting first: first
Expecting second: first
What we want to be printed to the console:
Expecting first: first
Expecting second: second
But for reference, this is a simple scenario where multiple conflicting classes are concatenated together:
e.g.
./src/first.js
export class Foo{
test(){
console.log('Expecting first:', Foo.value)
}
}
Foo.value = 'first'
./src/second.js:
export class Foo {
test(){
console.log('Expecting second:', Foo.value)
}
}
Foo.value = 'second'
with entry point ./src/index.js:
import { Foo } from './first'
import { Foo as SecondFoo } from './second'
new Foo().test() //expect 'first',
new SecondFoo().test() //expect 'second', actually get 'first'
will output:
// CONCATENATED MODULE: ./src/first.js
class Foo{
test(){
console.log('Expecting first:', Foo.value)
}
}
Foo.value = 'first'
// CONCATENATED MODULE: ./src/second.js
class second_js_Foo {
test(){
console.log('Expecting second:', Foo.value)
}
}
second_js_Foo.value = 'second'
// CONCATENATED MODULE: ./src/index.js
new Foo().test() //expect 'first',
new second_js_Foo().test() //expect 'second', actually get 'first'
What is the expected behavior?
The expected behaviour is to rename the reference inside the class method to correctly point at the new name of the class, e.g.
// CONCATENATED MODULE: ./src/second.js
class second_js_Foo {
test(){
console.log('Expecting second:', second_js_Foo.value)
}
}
second_js_Foo.value = 'second'
If this is a feature request, what is motivation or use case for changing the behavior?
Please mention other relevant information such as the browser version, Node.js version, webpack version and Operating System.
webpack next branch from github.
node 8.
The problem seems to be that when escope parses the AST, it correctly finds the references in the class methods that resolve to the class variable.
However, when ConcatenatedModule.js iterates over the module scope variables, it turns out these variables in the module scope have a different identity to the ones escope assigned the references to!
Thus the reference in the class method is not there when ConcatenatedModule goes to rename the the class variable.
It's not clear to me whether this is a bug in escope or a bug in ConcatenatedModule.js, but I managed to come up with a fix: recursively gather all the variables found in all the scopes inside the module scope, then add each lower-scope variable's references to the identically named higher scope variable.
This seems to work (even on a large, complex codebase), but I'm not sure if it's the right solution to the problem - it's also not ready to be merged (there are no test cases, for example) - could you please let me know what you think about the approach? Is it fixing the bug in the wrong place?
https://github.com/willheslam/webpack/tree/fix-renaming-bug-with-class-method
https://github.com/webpack/webpack/compare/next...willheslam:fix-renaming-bug-with-class-method?expand=1
If it's on the right track I can fix it up, adhere to the same style guide etc.
(As an extra aside, this feature is otherwise working really well so far - as well as trying it out on simple demos, I have managed to put a large, complex production app into webpack 3/next with dependencies like rxjs and immutablejs.
Combining the partial scope hoisting, tree shaking, ES6 versions of rxjs and immutablejs (not the transpiled and minified versions they publish) and using uglifyes (ES6-compatible version of uglifyjs), I've been able to create a pure ES6 bundle that is around 25% smaller than the ES5 bundle and evaluates roughly 15 - 20% faster... very exciting stuff!)