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

Scope hoisting renaming after babel transforms #2292

Merged
merged 3 commits into from
Nov 18, 2018

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Nov 17, 2018

↪️ Pull Request

With this change the renaming part of scope-hoisting seems to work with other babel-transforms as well. E.g.:

  1. Superclass identifiers get renamed correctly (closes Extending EventEmitter throws error in production bundle when using --experimental-scope-hoisting flag #2204)
  2. <span>Test</span> gets transpiled to React.createElement("span", null, "Test"), but React isn't getting renamed (same for h of preact, hyperapp). (closes Tree shaking and React #1699, closes Scope hoisting/tree shaking bug (--experimental-scope-hoisting) #2084)
  3. babel-plugin-lodash transforms the ast just like jsx (closes babel lodash plugin and parcel tree shaking #1821)
  4. most things regarding constantViolations undefined: with styled-jsx - closes Experimental scope hoisting: Cannot read property 'constantViolations' of undefined #1538, with plugin-transform-runtime - closes scope hoisting: Cannot read property 'constantViolations' of undefined #2255

I've added test for jsx and superclasses to the scope-hoisting/es6 folder.

Is a reason for the custom renaming logic? All scope-hoisting tests still pass with simply scope.rename(oldName, newName).

💻 Examples

Previously, scope-hoisting didn't correctly rename the superclass identifier (same problem with jsx)

src:

import Component from "./x.js";

console.log(Component);

class App extends Component {}

Before hoisting:

// ...

import Component from "./x.js";
console.log(Component);

var App =
/*#__PURE__*/
function (_Component) {
  _inherits(App, _Component);

  function App() {
    _classCallCheck(this, App);

    return _possibleConstructorReturn(this, _getPrototypeOf(App).apply(this, arguments));
  }

  return App;
}(Component);

After hoisting:

// ...

import $Focm$import$Component from "./x.js";
console.log($Focm$import$Component);

var App =
/*#__PURE__*/
function (_Component) {
  _inherits(App, _Component);

  function App() {
    _classCallCheck(this, App);

    return _possibleConstructorReturn(this, _getPrototypeOf(App).apply(this, arguments));
  }

  return App;
}(Component); // <--- Problem

@mischnic mischnic changed the title Scope hoisting rename Scope hoisting renaming Nov 17, 2018
@mischnic mischnic changed the title Scope hoisting renaming Scope hoisting renaming after babel transforms Nov 17, 2018
@mischnic mischnic mentioned this pull request Nov 18, 2018
@devongovett devongovett merged commit 0937705 into parcel-bundler:master Nov 18, 2018
@@ -51,6 +51,8 @@ function hasSideEffects(asset, {sideEffects} = asset._package) {
module.exports = {
Program: {
enter(path, asset) {
path.scope.crawl();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why we need this, isn't babel traverse going to .crawl for us already?

Copy link
Member Author

@mischnic mischnic Nov 18, 2018

Choose a reason for hiding this comment

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

Here is an explanation: #1699 (comment)

It seems that when JSX is transformed (calling babel.transformFromAst(asset.ast, asset.contents, config)), the scope binding is not updated (only AST is updated). So in the hoist phase, we fail to find binding of React because there is still JSX syntax in the scope.

Copy link
Member

Choose a reason for hiding this comment

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

I think babel doesn't recrawl the scope automatically, only when the scope is first initialized. That happens before lots of babel transforms run which can modify references, in this case the class transform. Then we get to our hoisting transform and we need to re-crawl to make sure we have updated references since babel traverse doesn't do that automatically.

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