Fix check for JSXMemberExpression deopt in react-constant-elements #4787

DrewML · 2016-10-27T04:15:24Z

Q	A
Bug fix?	yes
Breaking change?	no
New feature?	no
Deprecations?	no
Spec compliancy?	no
Tests added/pass?	yes
Fixed tickets	#4397
License	MIT
Doc PR	reference to the documentation PR, if any

This fixes the bug reported, but I'm not sure that it is the ideal fix.

Edit: The more I think about this, the more I'm thinking this may be a bug in PathHoister. It (correctly) does not hoist when you do const self = this; and then do <self.subComponent />. Will keep looking.

Open to any feedback/guidance.

…ments

codecov-io · 2016-10-27T04:23:55Z

Current coverage is 89.32% (diff: 100%)

No coverage report found for master at e1ac315.

Powered by Codecov. Last update e1ac315...1d45160

DrewML · 2016-11-02T03:13:02Z

Ok, the more I dig into this, the more I think this is probably the correct fix. It seems like it is only the job of PathHoister to do the hoisting, but it's the job of the caller to determine if the path it is dealing with is safe to hoist.

Feedback @hzoo @danez et al?

STRML · 2016-11-07T15:46:49Z

I don't think this will work properly - the existing path.isJSXMemberExpression() call on L19 should be kept, and the check on L14 doesn't actually work - isJSXMemberExpression(opts) will do a shallowEqual which will fail due to the nested object:

export function is(type: string, node: Object, opts?: Object): boolean {
  if (!node) return false;

  let matches = isType(node.type, type);
  if (!matches) return false;

  if (typeof opts === "undefined") {
    return true;
  } else {
    return t.shallowEqual(node, opts);
  }
}

This is closer, but doesn't catch some cases where we actually could hoist: https://astexplorer.net/#/qNI32z88dI/2

However, I think we consider fixing this in Hoister. Not being able to catch the this reference sure sounds like a bug.

gaearon · 2017-01-15T16:02:11Z

This seems like the last blocker to us enabling constant-elements by default.

DrewML · 2017-01-15T20:43:17Z

@gaearon I'm pretty blocked this week by some stuff at work, but I can take another look at it next weekend if someone else doesn't get to it first.

STRML · 2017-01-16T17:13:46Z

I'm going to take a look at it today. I believe the culprit is Hoister not treating references to this like a parameter, so it misses that the reference is no longer valid outside of the original scope.

The PathHoister ignored member references on "this", causing it to potentially hoist an expression above its function scope. This patch tells the hoister to watch for "this", and if seen, mark the nearest non-arrow function scope as the upper limit for hoistng. This fixes babel#4397 and is an alternative to babel#4787.

STRML · 2017-01-16T20:22:00Z

Please see #5143 for what I believe is a final fix that even catches the eager deopt in https://astexplorer.net/#/qNI32z88dI/2!

The PathHoister ignored member references on "this", causing it to potentially hoist an expression above its function scope. This patch tells the hoister to watch for "this", and if seen, mark the nearest non-arrow function scope as the upper limit for hoistng. This fixes babel#4397 and is an alternative to babel#4787.

The PathHoister ignored member references on "this", causing it to potentially hoist an expression above its function scope. This patch tells the hoister to watch for "this", and if seen, mark the nearest non-arrow function scope as the upper limit for hoistng. This fixes #4397 and is an alternative to #4787.

* Add new flow preset (#5288) * Fix PathHoister hoisting JSX member expressions on "this". (#5143) The PathHoister ignored member references on "this", causing it to potentially hoist an expression above its function scope. This patch tells the hoister to watch for "this", and if seen, mark the nearest non-arrow function scope as the upper limit for hoistng. This fixes #4397 and is an alternative to #4787. * Fix PathHoister hoisting before bindings. (#5153) Fixes #5149 and enables a few additional safe hoists. * Fix linting error * feature: Support pure expressions in transform-react-constant-elements (#4812) * Fix loose for-of with label (#5298) * Rewrite Hub as interface #5047 (#5050) * Rewrite Hub as interface #5047 * Update index.js * Avoid adding unnecessary closure for block scoping (#5246) When you write ``` for (const x of l) { setTimeout(() => x); } ``` we need to add a closure because the variable is meant to be block-scoped and recreated each time the block runs. We do this. However, we also add the closure when no loop is present. This isn't necessary, because if no loop is present then each piece of code runs at most once. I changed the transform to only add a closure if a variable is referenced from within a loop. * Add greenkeeperio-bot to mention-bot blacklist (#5301) [skip ci] * Upgrade lerna to current beta. (#5300) * Revert "Upgrade lerna to current beta." (#5303) * Add charset so tests work with convert-source-map@>1.4 (#5302) * Add CHANGELOG for 6.23.0 [skip ci] (#5304) * Update babel-types README from script. * v6.23.0 * Revert change that lerna force-committed. * Revert "Rewrite Hub as interface #5047" (#5306) * v6.23.1 * Revert lerna again

danez · 2017-03-16T17:38:49Z

@STRML @DrewML Is this PR now obsolete as #5143 is merged?

STRML · 2017-03-16T18:51:00Z

Correct - safe to close.

The PathHoister ignored member references on "this", causing it to potentially hoist an expression above its function scope. This patch tells the hoister to watch for "this", and if seen, mark the nearest non-arrow function scope as the upper limit for hoistng. This fixes #4397 and is an alternative to #4787.

Deopt on JSXMemberExpression with Object "this" in react-constant-ele…

1d45160

…ments

hzoo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Nov 15, 2016

appden mentioned this pull request Dec 4, 2016

Re-enable babel-plugin-transform-react-constant-elements when it’s not buggy facebook/create-react-app#553

Closed

babel-bot added PR: Bug Fix 🐛 A type of pull request used for our changelog categories and removed PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jan 5, 2017

STRML mentioned this pull request Jan 16, 2017

Fix PathHoister hoisting JSX member expressions on "this". #5143

Merged

STRML closed this Mar 16, 2017

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix check for JSXMemberExpression deopt in react-constant-elements #4787

Fix check for JSXMemberExpression deopt in react-constant-elements #4787

DrewML commented Oct 27, 2016 •

edited

codecov-io commented Oct 27, 2016 •

edited

DrewML commented Nov 2, 2016

STRML commented Nov 7, 2016 •

edited

gaearon commented Jan 15, 2017

DrewML commented Jan 15, 2017 •

edited

STRML commented Jan 16, 2017

STRML commented Jan 16, 2017

danez commented Mar 16, 2017

STRML commented Mar 16, 2017

Fix check for JSXMemberExpression deopt in react-constant-elements #4787

Fix check for JSXMemberExpression deopt in react-constant-elements #4787

Conversation

DrewML commented Oct 27, 2016 • edited

codecov-io commented Oct 27, 2016 • edited

Current coverage is 89.32% (diff: 100%)

DrewML commented Nov 2, 2016

STRML commented Nov 7, 2016 • edited

gaearon commented Jan 15, 2017

DrewML commented Jan 15, 2017 • edited

STRML commented Jan 16, 2017

STRML commented Jan 16, 2017

danez commented Mar 16, 2017

STRML commented Mar 16, 2017

DrewML commented Oct 27, 2016 •

edited

codecov-io commented Oct 27, 2016 •

edited

STRML commented Nov 7, 2016 •

edited

DrewML commented Jan 15, 2017 •

edited