Fix PathHoister hoisting JSX member expressions on "this". #5143
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
force-pushed
the
fix-react-constant-elements-this
branch
from
3dfd0fb
to
8724135
Compare
STRML
added
PR: Bug Fix 🐛
Current coverage is 89.21% (diff: 100%)@@ master #5143 diff @@
==========================================
Files 203 203
Lines 9828 9833 +5
Methods 1071 1071
Messages 0 0
Branches 2616 2619 +3
==========================================
+ Hits 8768 8773 +5
Misses 1060 1060
Partials 0 0
|
loganfsmyth
reviewed
Feb 13, 2017
| return; | ||
| } | ||
|
|
||
| // If the identifier refers to `this`, we need to break on the closest non-arrow scope. | ||
| if (path.node.name === "this") { |
There was a problem hiding this comment.
Oh gross, I didn't know JSX didn't use ThisExpression. I bet we have bugs if you transpile ES6 -> ES5 with JSX pass-through enabled. No way does everything that processes this also check for JSXIdentifier.
Nothing for you to change, just complaining :P
loganfsmyth
approved these changes
Feb 13, 2017
danez
added a commit
that referenced
this pull request
Feb 14, 2017
* 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
existentialism
pushed a commit
that referenced
this pull request
May 19, 2017
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.
lock
bot
added
the
outdated
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.