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

Use regenerator-transform to implement babel-plugin-transform-regenerator #4881

Merged
merged 3 commits into from
Dec 8, 2016

Conversation

benjamn
Copy link
Contributor

@benjamn benjamn commented Nov 21, 2016

Q A
Bug fix? no
Breaking change? no
New feature? no
Deprecations? no
Spec compliancy? no
Tests added/pass? no/yes
Fixed tickets
License MIT
Doc PR no
Dependency Changes yes

I'm very happy to say I finally got around to finishing the work that @kittens prototyped in facebook/regenerator#222.

Just as babel-runtime now depends on the external regenerator-runtime package, I believe it makes sense for babel-plugin-transform-regenerator to become a thin wrapper around regenerator-transform, a pure-Babel plugin implementing exactly the same logic as before, without any extraneous dependencies like defs, recast, or ast-types.

Though I believe this pull request is safe to merge, you should probably wait to merge it until facebook/regenerator#259 is merged. That pull request also provides more explanation of why I think moving the implementation of Regenerator back to its original repository is a good idea. I'm happy to compromise on anything necessary to make this happen.

Note that the regenerator-transform version is pegged to an exact version (0.9.7). Past experience has taught me that releasing new versions of Regenerator is almost impossible if client projects are allowed to pull in new patch versions automatically. I will happily submit a pull request whenever a new version is released.

@benjamn
Copy link
Contributor Author

benjamn commented Nov 21, 2016

Note that TEST_GREP=regenerator make test-only (10 tests) still passes, in addition to the numerous tests over at https://github.com/facebook/regenerator/tree/master/test.

@codecov-io
Copy link

codecov-io commented Nov 21, 2016

Current coverage is 89.14% (diff: 100%)

No coverage report found for master at 723c90e.

Powered by Codecov. Last update 723c90e...4101dc9

@benjamn
Copy link
Contributor Author

benjamn commented Nov 30, 2016

facebook/regenerator#259 has now been merged, so I believe this PR is ready for serious consideration!

@benjamn
Copy link
Contributor Author

benjamn commented Dec 1, 2016

Here are three bugs that I've fixed in https://github.com/facebook/regenerator since merging facebook/regenerator#259:

These bugs would also be fixed in Babel when/if this PR gets merged!

@hzoo hzoo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Dec 1, 2016
@hzoo
Copy link
Member

hzoo commented Dec 1, 2016

What should we do about tests, etc? Right now we are just going to be calling out to regenerator-transform. I guess we can delete .test at least

Also realized npmignore didn't ignore .test or src folders

@benjamn
Copy link
Contributor Author

benjamn commented Dec 1, 2016

I think the tests in https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-regenerator/test are still useful as integration tests, but .test should be deleted (will do).

…ator.

Though I believe this is safe to merge, you may want to wait to merge it
until facebook/regenerator#259 is merged. That
pull request also provides more explanation of why I think moving the
implementation of Regenerator back to its original repository is a good
idea. I'm happy to compromise on anything necessary to make this happen.

Note that the regenerator-transform version is pegged to an exact version
(0.9.7). Past experience has taught me that releasing new versions of
Regenerator is almost impossible if client projects are allowed to pull in
new patch versions automatically. I will happily submit a pull request
whenever a new version is released.
@hzoo
Copy link
Member

hzoo commented Dec 2, 2016

Cool 😄

export default function () {
return require("./visit");
}
export default require("regenerator-transform");
Copy link
Member

@hzoo hzoo Dec 8, 2016

Choose a reason for hiding this comment

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

Looking at https://unpkg.com/[email protected]/lib/index.js

I think due to module.exports = exports["default"]; this needs to be .default? Although not sure why there aren't any failing tests

or we use import and the export it? @benjamn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about export { default } from "regenerator-transform"? Or even export * from "regenerator-transform"?

@hzoo hzoo merged commit 16c84fb into babel:master Dec 8, 2016
@hzoo
Copy link
Member

hzoo commented Dec 8, 2016

Alright let's do it then 🎉 thanks for taking this on @benjamn

Jessidhia pushed a commit to Jessidhia/babel that referenced this pull request Dec 12, 2016
* master:
  make installing runtime/transform-runtime clearer [skip ci] (babel#4991)
  Add example to es2015-unicode-regex [skip ci] (babel#4983)
  v6.20.3
  Calculate the correct arity for async functions with destructuring - fixes babel#4977 (babel#4978)
  v6.20.2
  fix object spread (babel#4976)
  fix clean lib
  update readme [skip ci]
  v6.20.1
  Fix nested object spread (babel#4974)
  v6.20.0
  v6.20.0 changelog [skip ci] (babel#4971)
  Raise limit on code size before compacting (babel#4965)
  Use regenerator-transform to implement babel-plugin-transform-regenerator (babel#4881)
  Add getBindingIdentifierPaths/getOuterBindingIdentifierPaths (babel#4876)
  Hoist generateDeclaredUidIdentifier helper function (babel#4934)
Jessidhia pushed a commit to Jessidhia/babel that referenced this pull request Dec 16, 2016
* master: (89 commits)
  fix sizing [skip ci]
  typo [skip ci]
  changelog for v6.20.1 to v6.20.3 [skip ci]
  mention repl/link [skip ci]
  add semver label [skip ci]
  Fix links in CONTRIBUTING.md [skip ci] (babel#4989)
  make installing runtime/transform-runtime clearer [skip ci] (babel#4991)
  Add example to es2015-unicode-regex [skip ci] (babel#4983)
  v6.20.3
  Calculate the correct arity for async functions with destructuring - fixes babel#4977 (babel#4978)
  v6.20.2
  fix object spread (babel#4976)
  fix clean lib
  update readme [skip ci]
  v6.20.1
  Fix nested object spread (babel#4974)
  v6.20.0
  v6.20.0 changelog [skip ci] (babel#4971)
  Raise limit on code size before compacting (babel#4965)
  Use regenerator-transform to implement babel-plugin-transform-regenerator (babel#4881)
  ...
panagosg7 pushed a commit to panagosg7/babel that referenced this pull request Jan 17, 2017
…ator (babel#4881)

* Use regenerator-transform to implement babel-plugin-transform-regenerator.

Though I believe this is safe to merge, you may want to wait to merge it
until facebook/regenerator#259 is merged. That
pull request also provides more explanation of why I think moving the
implementation of Regenerator back to its original repository is a good
idea. I'm happy to compromise on anything necessary to make this happen.

Note that the regenerator-transform version is pegged to an exact version
(0.9.7). Past experience has taught me that releasing new versions of
Regenerator is almost impossible if client projects are allowed to pull in
new patch versions automatically. I will happily submit a pull request
whenever a new version is released.

* Remove never-used babel-plugin-transform-regenerator/.test directory.

* regenerator-transform to 0.9.8
@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: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants