Skip to content

Do not hoist jsx referencing a mutable binding#10529

Merged
nicolo-ribaudo merged 1 commit intobabel:masterfrom
nicolo-ribaudo:issue-10522
Oct 8, 2019
Merged

Do not hoist jsx referencing a mutable binding#10529
nicolo-ribaudo merged 1 commit intobabel:masterfrom
nicolo-ribaudo:issue-10522

Conversation

@nicolo-ribaudo
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo commented Oct 7, 2019

Q                       A
Fixed Issues? Fixes #10522, fixes #8310
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Related tests:

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: traverse area: jsx labels Oct 7, 2019

// we can handle reassignments only if they happen in the same scope as the declaration
for (const violation of binding.constantViolations) {
if (violation.scope !== binding.path.scope) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can't we still hoist to the level of the violation?

let foo = 'hello';
export const Component = () => {
  foo = 'goodbye';
  return <span>{array.map(() => <inner>{foo}</inner>}</span>;
};

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, we can't. Consider this example:

let foo = 0;
export const Component = () => {
  foo++;
  return Promise.resolve().then(() => <span>{foo}</span>);
};

Component();
Component();

Here both the components should use 2, but if we hoist it the first one will use 1.

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

Labels

area: jsx outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: traverse PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

transform-react-constant-elements: using mutable variable doesn't lead to deoptimization Components get hoisted outside of valid scope

4 participants