This repository was archived by the owner on Feb 1, 2025. It is now read-only.
Register new bindings when hoisting vars#377
Merged
benjamn merged 1 commit intofacebook:masterfrom Jul 19, 2019
Merged
Conversation
|
@benjamn |
benjamn
approved these changes
Jul 19, 2019
Contributor
benjamn
left a comment
There was a problem hiding this comment.
Thanks as always @nicolo-ribaudo!
|
Testet it. In case of babel/10193 it works like a charm. |
This file contains hidden or 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
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.
In #368, I changed
regenerator-transformto remove bindings which are hoisted, since it caused problems when they were registered again.Nothing in this plugin actually registered them, but sometimes Babel re-traverses the scope and registers declaration which weren't already registerd (this usually happens when a function node is discarded and re-created, I think). For this reason, this code used to throw:
because when Babel did re-crawl the bindings, it already had a
tmpbinding registered (forconst tmp) and tried to register a new one with the same name (for the hoistedvar tmp).Unfortunately we can't always rely on something to trigger the scope update, so simply deleting the old binding isn't enough. We also need to ensure that the new hoisted
vars get registered again. Otherwise, we cause conflicts with thetransform-modules-commonjsplugin (or any other plugin relying on scope information):This code is initially transformed by regenerator to something like this:
it correctly removes the
const someActionbinding from the scope, but before this PR it didn't register thevar someActionbinding. For this reason, the modules transform plugin thought thatsomeAction = barwas assigning to the imported binding (which is read-only), generating an error.Fixes babel/babel#10193
PS: Do you have a custom
.npmrcfor this project on your machine? Every time I runnpm testit generatespackage-lock.jsonfiles for the subpackages, and it changes thedependenciessection in the top-levelpackage.jsonfile to reference the local packages.