Skip to content
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
nicolo-ribaudo:babel-issue-10193
Jul 19, 2019
Merged

Register new bindings when hoisting vars#377
benjamn merged 1 commit intofacebook:masterfrom
nicolo-ribaudo:babel-issue-10193

Conversation

@nicolo-ribaudo
Copy link
Copy Markdown
Contributor

@nicolo-ribaudo nicolo-ribaudo commented Jul 15, 2019

In #368, I changed regenerator-transform to 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:

async function foo() {
	(async function (number) {
		const tmp = number
	})
}

because when Babel did re-crawl the bindings, it already had a tmp binding registered (for const tmp) and tried to register a new one with the same name (for the hoisted var 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 the transform-modules-commonjs plugin (or any other plugin relying on scope information):

import { someAction } from 'actions';

function* foo() {
  const someAction = bar;
}

This code is initially transformed by regenerator to something like this:

import { someAction } from 'actions';

function foo() {
  var someAction;

  return regeneratorRuntime.wrap(function foo() {
    someAction = bar;
  });
}

it correctly removes the const someAction binding from the scope, but before this PR it didn't register the var someAction binding. For this reason, the modules transform plugin thought that someAction = bar was assigning to the imported binding (which is read-only), generating an error.

Fixes babel/babel#10193


PS: Do you have a custom .npmrc for this project on your machine? Every time I run npm test it generates package-lock.json files for the subpackages, and it changes the dependencies section in the top-level package.json file to reference the local packages.

@pwnreza
Copy link
Copy Markdown

pwnreza commented Jul 19, 2019

@benjamn
sry for the possibly dumb question but my team and I are waiting for this commit.
So we wanted to ask when we could expect this to be released with a new version.

Copy link
Copy Markdown
Contributor

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

Thanks as always @nicolo-ribaudo!

@benjamn benjamn merged commit ffc74cc into facebook:master Jul 19, 2019
@nicolo-ribaudo nicolo-ribaudo deleted the babel-issue-10193 branch July 19, 2019 16:04
@pwnreza
Copy link
Copy Markdown

pwnreza commented Jul 19, 2019

Testet it. In case of babel/10193 it works like a charm.
Big thanks to @nicolo-ribaudo

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Variable assignment while object destructuring not working properly 'Error: "action" is read-only.'

4 participants