Skip to content

Avoid adding unnecessary closure for block scoping - #5246

Merged
loganfsmyth merged 1 commit into
babel:masterfrom
sophiebits:block-no-unnecessary-functions
Feb 13, 2017
Merged

Avoid adding unnecessary closure for block scoping#5246
loganfsmyth merged 1 commit into
babel:masterfrom
sophiebits:block-no-unnecessary-functions

Conversation

@sophiebits

@sophiebits sophiebits commented Jan 31, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.)

@sophiebits
sophiebits force-pushed the block-no-unnecessary-functions branch from 48a0bde to 3d73fca Compare January 31, 2017 01:00
@sophiebits sophiebits mentioned this pull request Jan 31, 2017
Closed
@codecov-io

codecov-io commented Jan 31, 2017 •
edited
Loading

Copy link
Copy Markdown

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

sophiebits commented Feb 6, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

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

wtgtybhertgeghgtwtg commented Feb 9, 2017

Copy link
Copy Markdown
Contributor

Is this gonna make it into babel@7.0.0?

@loganfsmyth

loganfsmyth commented Feb 9, 2017

Copy link
Copy Markdown
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.

@sophiebits
Avoid adding unnecessary closure for block scoping
7f9af76 
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.
@sophiebits
sophiebits force-pushed the block-no-unnecessary-functions branch from 3d73fca to 7f9af76 Compare February 11, 2017 00:51
@loganfsmyth
loganfsmyth merged commit 14d3c2e into babel:master Feb 13, 2017
@hzoo

hzoo commented Feb 13, 2017

Copy link
Copy Markdown
Member

Thanks again @spicyj 👌

@sophiebits

sophiebits commented Feb 13, 2017

Copy link
Copy Markdown
Contributor Author

Thank you all!!

danez added a commit that referenced this pull request Feb 14, 2017
@danez
Merge master into 7.0 (#5310)
266332a 
* 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
@xtuc xtuc mentioned this pull request Feb 28, 2017
Merged
existentialism pushed a commit that referenced this pull request May 19, 2017
@sophiebits @existentialism
Avoid adding unnecessary closure for block scoping (#5246)
f57e420 
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.

Reviewers

@loganfsmyth loganfsmyth loganfsmyth approved these changes
+1 more reviewer
@xtuc xtuc xtuc approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area: perf outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sophiebits @codecov-io @wtgtybhertgeghgtwtg @loganfsmyth @hzoo @xtuc