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

Add option to block-scoping to throw on slow code #5236

Merged
merged 1 commit into from Feb 6, 2017

Conversation

sophiebits
Copy link
Contributor

The let/const plugin can add closures where you don't expect them. This is undesirable in some perf-sensitive projects (ex: React). I added an option that throws whenever the plugin adds a function (as opposed to simply renaming variables when converting to var).

Q A
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? yes
Deprecations?
Spec Compliancy?
Tests Added/Pass? yes
Fixed Tickets
License MIT
Doc PR
Dependency Changes

@mention-bot
Copy link

@spicyj, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo, @ryb73 and @jayphelps to be potential reviewers.

@hzoo hzoo changed the title Add option to block-scoping to slow on throw code Add option to block-scoping to throw on slow code Jan 28, 2017
@hzoo hzoo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Jan 28, 2017
@codecov-io
Copy link

codecov-io commented Jan 28, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@2104ab6).

@@            Coverage Diff            @@
##             master    #5236   +/-   ##
=========================================
  Coverage          ?   89.23%           
=========================================
  Files             ?      203           
  Lines             ?     9844           
  Branches          ?     2625           
=========================================
  Hits              ?     8784           
  Misses            ?     1060           
  Partials          ?        0
Impacted Files Coverage Δ
...plugin-transform-es2015-block-scoping/src/index.js 94.26% <100%> (ø)

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 2104ab6...53e694c. Read the comment docs.

Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

Approach looks fine to me - Dono how much we want to change the error message is all?

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

👍

@hzoo
Copy link
Member

hzoo commented Jan 28, 2017

Actually can you also update the readme for this option? (wish that check could be automated)

Can use https://github.com/babel/babel/blob/master/packages/babel-plugin-transform-es2015-classes/README.md#usage as a reference (usage/options explanation)

@sophiebits sophiebits force-pushed the block-throw-slow branch 2 times, most recently from 85e52aa to 27b7dc6 Compare January 28, 2017 21:35
@sophiebits
Copy link
Contributor Author

README updated.

@sophiebits
Copy link
Contributor Author

Happy to change the error message too if you like. It feels a little long to me.

@@ -15,9 +15,19 @@ npm install --save-dev babel-plugin-transform-es2015-block-scoping
**.babelrc**

```json
// without options
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, but since were using json now, can we move the without options outside like in https://github.com/babel/babel/pull/5194/files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(y)

@hzoo
Copy link
Member

hzoo commented Jan 28, 2017

Cool 👍 maybe cc @gaearon for suggestions as well

@babel-bot
Copy link
Collaborator

Hey @spicyj! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

@hzoo
Copy link
Member

hzoo commented Jan 28, 2017

Can ignore ^, looks like timeout issues for babel-cli?

@sophiebits
Copy link
Contributor Author

^^ I assume these timeouts are random?

  1669 passing (3m)
  7 pending
  5 failing
  1) bin/babel --ignore:
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
  
  2) bin/babel --ignore complete:
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
  
  3) bin/babel-node --print:
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
  
  4) bin/babel-node directory:
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
  
  5) bin/babel-node filename:
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

@existentialism
Copy link
Member

What about:

Rewriting the variable declarations in this block will add a closure (throwIfClosureRequired).

@glenjamin
Copy link

Excuse my ignorance, but are there any notes around on why a function is required over variable renaming in some cases?

@ljharb
Copy link
Member

ljharb commented Jan 28, 2017

@glenjamin the only way to avoid a function in the loop case is by loop unrolling, which can only be done when all iteration variable values can be statically known.

@sophiebits
Copy link
Contributor Author

However, there are cases, like with a forEach rather than a setTimeout, where the closure behavior is not necessary and the code can be written in a different way without.

@ljharb
Copy link
Member

ljharb commented Jan 28, 2017

Indeed - in that case the forEach already takes a function, so there's already a closure available.

@Jessidhia
Copy link
Member

@ljharb when it escapes, there is no way for the transform to know if the closure call is sync or async. If there is any chance of it being async, the block needs to go through a closure layer to save the value.

We could hardcode some names but that would still be a trap. For example, while Array.prototype.map is sync, require('bluebird').prototype.map isn't.

@@ -14,12 +14,24 @@ npm install --save-dev babel-plugin-transform-es2015-block-scoping

**.babelrc**

Without options:
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a new line before the code block, otherwise it won't be formatted correctly on the website.

Copy link
Member

Choose a reason for hiding this comment

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

Addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just copying https://github.com/babel/babel/pull/5194/files like I was asked. :) Updated and cut the error message down a bit.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, sorry. I think GH collapsed that comment and I only saw the newer one. 😞

Copy link
Member

Choose a reason for hiding this comment

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

@spicyj yeah. I'm sure we had issue with it.

Thanks for the update :)

The let/const plugin can add closures where you don't expect them. This is undesirable in some perf-sensitive projects (ex: React). I added an option that throws whenever the plugin adds a function (as opposed to simply renaming variables when converting to var).
sophiebits added a commit to sophiebits/react that referenced this pull request Jan 30, 2017
Test Plan: Patch in babel/babel#5236, run `grunt build:modules` with no errors.
@sophiebits
Copy link
Contributor Author

Is this good to merge?

@hzoo hzoo merged commit ff8a10e into babel:master Feb 6, 2017
@hzoo
Copy link
Member

hzoo commented Feb 6, 2017

Thanks @spicyj! 🙂

@sophiebits
Copy link
Contributor Author

Thank you!

existentialism pushed a commit that referenced this pull request May 19, 2017
The let/const plugin can add closures where you don't expect them. This is undesirable in some perf-sensitive projects (ex: React). I added an option that throws whenever the plugin adds a function (as opposed to simply renaming variables when converting to var).
@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
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet