Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid adding unnecessary closure for block scoping #5246

Merged
merged 1 commit into from Feb 13, 2017

Conversation

sophiebits
Copy link
Contributor

@sophiebits sophiebits commented Jan 31, 2017

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 a block is present but no loop is:

function foo() {
  {
    const x;
    setTimeout(() => x);
  }
}

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.

(Builds on #5236.)

@codecov-io
Copy link

codecov-io commented Jan 31, 2017

Codecov Report

Merging #5246 into master will decrease coverage by -0.2%.

@@           Coverage Diff            @@
##           master   #5246     +/-   ##
========================================
- Coverage    89.4%   89.2%   -0.2%     
========================================
  Files         204     203      -1     
  Lines        9916    9851     -65     
  Branches     2670    2628     -42     
========================================
- Hits         8865    8788     -77     
- Misses       1051    1063     +12
Impacted Files Coverage Δ
...plugin-transform-es2015-block-scoping/src/index.js 93.45% <100%> (-0.85%)
packages/babel-register/src/cache.js 27.27% <ø> (-54.55%)
packages/babel-traverse/src/path/family.js 84.37% <ø> (-4.42%)
packages/babel-traverse/src/path/replacement.js 79.62% <ø> (-1.41%)
packages/babel-traverse/src/path/modification.js 85.43% <ø> (-0.68%)
packages/babel-traverse/src/path/evaluation.js 77.15% <ø> (-0.57%)
packages/babel-traverse/src/index.js 93.47% <ø> (-0.28%)
...bel-plugin-transform-es2015-classes/src/vanilla.js 95.67% <ø> (-0.18%)
packages/babel-generator/src/node/parentheses.js 92.24% <ø> (-0.14%)
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75ac320...7f9af76. Read the comment docs.

@sophiebits
Copy link
Contributor Author

sophiebits commented Feb 6, 2017

cc @gaearon @hzoo @existentialism – Any thoughts on this?

@hzoo hzoo added the area: perf label Feb 6, 2017
@wtgtybhertgeghgtwtg
Copy link
Contributor

Is this gonna make it into babel@7.0.0?

@loganfsmyth
Copy link
Member

This will land in 6, I'm investigating one last issue before we publish the next 6.x minor, so next few days probably.

7.x likely won't be for a few more weeks as a minimum, we've still got things we want to address for that.

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.
@loganfsmyth loganfsmyth merged commit 14d3c2e into babel:master Feb 13, 2017
@hzoo
Copy link
Member

hzoo commented Feb 13, 2017

Thanks again @spicyj 👌

@sophiebits
Copy link
Contributor Author

Thank you all!!

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
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.
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: perf outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants