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

Update regenerator-transform to new version #5567

Merged
merged 2 commits into from Apr 6, 2017

Conversation

aickin
Copy link
Contributor

@aickin aickin commented Mar 31, 2017

Q A
Patch: Bug Fix? yes
Major: Breaking Change? no
Minor: New Feature? no
Deprecations? no
Spec Compliancy? yes
Tests Added/Pass? yes
Fixed Tickets don't know of any filed
License MIT
Doc PR no
Dependency Changes updated regenerator-transform version

There was a bug with the code produced by regenerator-transform when it runs on an object with a shorthand property generator; the code produced by the transform created an undefined variable. In normal usage in Babel, this was usually masked by the fact that almost everyone uses babel-plugin-transform-regenerator after babel-plugin-transform-es2015-shorthand-properties, so the regenerator transformation was never seeing untransformed shorthand properties. (Also, shorthand property generators are not super common.)

I reported this problem in facebook/regenerator#267, and it was merged and released to npm in February. This PR updates babel-plugin-transform-regenerator to depend on that new version or regenerator-transform, and adds a unit test that fails with the old version of regenerator-transform.

Thanks for all the great work you do for the web and the world!

@mention-bot
Copy link

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

@aickin aickin changed the title Updating regenerator-transform Update regenerator-transform to new version Mar 31, 2017
@existentialism
Copy link
Member

existentialism commented Mar 31, 2017

@aickin we bumped this in 7.x via #5341, though we might want to consider back porting to 6.x?

@aickin
Copy link
Contributor Author

aickin commented Mar 31, 2017

@existentialism Sorry, I made a mistake, I was trying to PR into 6.x, and I didn't see that the default branch was 7.0. My mistake. I'll close this one and try to re-open a new one I guess?

@aickin aickin closed this Mar 31, 2017
@existentialism
Copy link
Member

existentialism commented Mar 31, 2017

No need to apologize! You can also change the base branch:

image

@aickin aickin changed the base branch from 7.0 to 6.x March 31, 2017 05:31
@aickin
Copy link
Contributor Author

aickin commented Mar 31, 2017

@existentialism Huh, didn't know you could do that in the github UI. Thanks!

I ran tests locally on top of 7.0, but if CI finds a problem, I'll fix it.

ETA: looks like tests passed both locally and in CI. Yay!

@aickin aickin reopened this Mar 31, 2017
@codecov
Copy link

codecov bot commented Mar 31, 2017

Codecov Report

Merging #5567 into 6.x will decrease coverage by 0.98%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              6.x    #5567      +/-   ##
==========================================
- Coverage   85.47%   84.48%   -0.99%     
==========================================
  Files         200      204       +4     
  Lines        9506     9599      +93     
  Branches     2702     2696       -6     
==========================================
- Hits         8125     8110      -15     
- Misses        886     1002     +116     
+ Partials      495      487       -8
Impacted Files Coverage Δ
...ges/babel-plugin-transform-decorators/src/index.js 9.43% <0%> (-79.14%) ⬇️
packages/babel-core/src/transformation/pipeline.js 41.37% <0%> (-21.59%) ⬇️
packages/babel-cli/src/babel/dir.js 58.69% <0%> (-9.6%) ⬇️
packages/babel-register/src/node.js 76.56% <0%> (-6.78%) ⬇️
packages/babel-generator/src/generators/types.js 93.58% <0%> (-6.42%) ⬇️
packages/babel-generator/src/generators/classes.js 94.82% <0%> (-5.18%) ⬇️
...ckages/babel-plugin-transform-runtime/src/index.js 75.4% <0%> (-4.6%) ⬇️
...-helper-transform-fixture-test-runner/src/index.js 83.52% <0%> (-4.59%) ⬇️
...l-plugin-transform-es2015-modules-amd/src/index.js 81.48% <0%> (-2.65%) ⬇️
...bel-plugin-transform-react-jsx-source/src/index.js 77.77% <0%> (-2.23%) ⬇️
... and 100 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 d349734...66f8546. Read the comment docs.

@aickin
Copy link
Contributor Author

aickin commented Mar 31, 2017

Hey Babel folks,

I'm at a loss on this code coverage report saying that this commit makes coverage go down in 100+ files. That doesn't make a lot of sense to me, given that I didn't change any Babel product code, and I added a test. Any help/guidance you could provide would be great; thanks!

@yavorsky
Copy link
Member

yavorsky commented Mar 31, 2017

Actually, #5341 initially was targeted to the master (6.0).
I don't see changes in 0.9.11 related only to 7.0, so it's LGTM.

Copy link
Member

@yavorsky yavorsky left a comment

Choose a reason for hiding this comment

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

Thanks!
It's good also to cover this case - facebook/regenerator#277 since it includes changes provided in 0.9.9. Or I could make a separate PR. 👌

@danez
Copy link
Member

danez commented Mar 31, 2017

Also maybe facebook/regenerator#283 which is for this issue: #4219

@danez
Copy link
Member

danez commented Apr 6, 2017

I added a test case to this PR for #4219.

@hzoo hzoo merged commit 3534bc8 into babel:6.x Apr 6, 2017
@hzoo hzoo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Apr 7, 2017
@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
6.x: backport outdated A closed issue/PR that is archived due to age. Recommended to make a new issue 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.

None yet

6 participants