You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.
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?
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!)
The text was updated successfully, but these errors were encountered:
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
What is currently printed to the console:
What we want to be printed to the console:
But for reference, this is a simple scenario where multiple conflicting classes are concatenated together:
e.g.
./src/first.js
./src/second.js:
with entry point ./src/index.js:
will output:
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.
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!)
The text was updated successfully, but these errors were encountered: