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

Webpack 3/next with partial scope hoisting does not rename class references in methods #5000

Closed
willheslam opened this issue Jun 4, 2017 · 1 comment · May be fixed by estools/escope#120

Comments

@willheslam
Copy link

willheslam commented Jun 4, 2017

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

  1. yarn install
  2. yarn build
  3. 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!)

@sokra
Copy link
Member

sokra commented Jun 5, 2017

It's a escope bug. Fix in PR. Feel free to continue testing with my PR branch. Results sounds great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants