Fix react constant elements bindings #5153
Merged
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
STRML
added
area: react
PR: Bug Fix 🐛
Codecov Report
@@ Coverage Diff @@
## master #5153 +/- ##
=========================================
Coverage ? 89.22%
=========================================
Files ? 203
Lines ? 9847
Branches ? 2626
=========================================
Hits ? 8786
Misses ? 1061
Partials ? 0
Continue to review full report at Codecov.
|
Fixes babel#5149 and enables a few additional safe hoists.
STRML
force-pushed
the
fix-react-constant-elements-bindings
branch
from
e3bc035
to
3a83715
Compare
|
Confirmed this fixes #5149 and that should be the last of the constant-elements issues! |
|
Wow, that's exciting! After this fix, have you tried enabling it on a large codebase (just to get a sense for any other remaining potential issues?) |
loganfsmyth
approved these changes
Feb 13, 2017
|
@gaearon We have not used this exact code in production yet, but our application does not hit any of the edge cases that this or the other PRs address, so we have been happily running previous constant-elements releases for many months now in production. |
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
Fixes #5149 and enables a few additional safe hoists.
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.
This relates to #3596, which fixed hoisting to certain positions where referenced bindings weren't evaluated yet, but was not able to handle moving a hoisted binding forward if we were already at Program scope.
Note that this actually enables a few more hoists, thus the few changed tests.