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

Remove @babel/plugin-transform-regenerator #789

Closed
wants to merge 4 commits into from

Conversation

tido64
Copy link
Contributor

@tido64 tido64 commented Mar 11, 2022

## Summary

Hermes added support for generators in 0.2.0, and JSC is ES6 feature complete since r202125.

Test plan

n/a

Hermes added support for generators in 0.2.0, and JSC is ES6 feature
complete since r202125.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 11, 2022
@tido64
Copy link
Contributor Author

tido64 commented Mar 11, 2022

@motiz88: It'd be great if you could make sure this doesn't break Meta internally.

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Mar 11, 2022
@motiz88
Copy link
Contributor

motiz88 commented Mar 11, 2022

Awesome - can you please run Prettier, remove @babel/plugin-transform-regenerator from package.json and update yarn.lock accordingly?

@facebook-github-bot
Copy link
Contributor

@motiz88 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@motiz88
Copy link
Contributor

motiz88 commented Mar 11, 2022

Looking good so far - I'm fixing up some minor Meta-internal things and planning to land this on Monday. Thanks for this, @tido64!

@newyankeecodeshop
Copy link
Contributor

This is great. I believe you can also remove babel-plugin-transform-for-of since iteration is also supported in both runtimes. Hopefully the minifier supports the syntax too.

@motiz88
Copy link
Contributor

motiz88 commented Mar 11, 2022

Hopefully the minifier supports the syntax too.

That is a good point - I'm not actually sure we have a test that would fail if it didn't. @tido64 would you mind checking to see what happens if you run this PR with minify=true? (cc @rh389 re: minifier stuff)

@robhogan
Copy link
Contributor

uglify-es and terser both fully support ES6 so I wouldn’t expect any issue for generators or for/of. uglify-es starts to get ugly for ES7+, which is part of the reason I’m looking to shift the default to terser.

@tido64
Copy link
Contributor Author

tido64 commented Mar 14, 2022

image

@motiz88: Something's failing internally. Is it because of this change?

This is great. I believe you can also remove babel-plugin-transform-for-of since iteration is also supported in both runtimes.

That's a great idea, but I think it should be a separate PR.

Hopefully the minifier supports the syntax too.

That is a good point - I'm not actually sure we have a test that would fail if it didn't. @tido64 would you mind checking to see what happens if you run this PR with minify=true? (cc @rh389 re: minifier stuff)

How do I turn this on in tests? I tried pasting some of the snapshots in Terser's REPL and Uglify's demo. Terser is fine with the generator function, but UglifyJS doesn't support it. I know uglify-es is supposed to support ES6, but it's deprecated and I see that people are trying to migrate to the former: #661

Is this going to be a problem?

@robhogan
Copy link
Contributor

robhogan commented Mar 14, 2022

I know uglify-es is supposed to support ES6, but it's deprecated and I see that people are trying to migrate to the former: #661

There's a bit of history here - uglify-js originally didn't support ES6. The author created uglify-es (originally a harmony branch of mishoo/UglifyJS2) to address that, but that was itself deprecated some time ago in favour of uglify-js, as uglify-js v3 does now support ES6 out of the box. I suspect that demo is just out of date. They all have CLIs:

echo "function* myGenerator(myVar) { yield myVar + 1; }" | npx uglify-js -m
function*myGenerator(e){yield e+1}

➜ echo "function* myGenerator(myVar) { yield myVar + 1; }" | npx terser -m
function*myGenerator(e){yield e+1}

➜ echo "function* myGenerator(myVar) { yield myVar + 1; }" | npx uglify-es -m
function*myGenerator(e){yield e+1}

Terser was originally a fork of mishoo/UglifyJS2 so it has a shared ancestry with uglify-es, and internally we've found it to be closer to uglify-es in output than uglify-js@3 is (#661 turned out to be problematic internally), so we're working to make terser the metro default and we'll probably deprecate metro-minifier-uglify.

TL;DR: Nothing minifier-related to worry about for this PR.

@robhogan
Copy link
Contributor

Something's failing internally. Is it because of this change?

Unrelated - React Native iOS tests are broken at the moment.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Mar 15, 2022
Summary:
## Summary

Hermes added support for generators in 0.2.0, and JSC is ES6 feature complete since r202125.

NOTE: This is a combined Metro + React Native commit. For React Native, the only change here is adding `babel/plugin-transform-regenerator` to `repo-config` ( = `devDependencies`), so the tests no longer implicitly consume it via Metro.

Changelog: [Internal]

X-link: facebook/metro#789

Reviewed By: rh389

Differential Revision: D34819044

Pulled By: motiz88

fbshipit-source-id: fe27b2c2af0281d5309d4c16f95762d4d4935eba
@tido64 tido64 deleted the tido/remove-regenerator branch March 15, 2022 12:38
facebook-github-bot pushed a commit that referenced this pull request Mar 17, 2022
Summary:
## Summary

As a follow-up to #789 , this PR removes `babel/plugin-transform-for-of` from the preset for JavaScriptCore users.

Pull Request resolved: #792

Test Plan: All unit tests pass

Reviewed By: motiz88

Differential Revision: D34927434

Pulled By: rh389

fbshipit-source-id: d38cd529c371ca0e13071f26c00d3a322ff5fab6
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…crosoft#789)

Summary:
## Summary

Hermes added support for generators in 0.2.0, and JSC is ES6 feature complete since r202125.

NOTE: This is a combined Metro + React Native commit. For React Native, the only change here is adding `babel/plugin-transform-regenerator` to `repo-config` ( = `devDependencies`), so the tests no longer implicitly consume it via Metro.

Changelog: [Internal]

X-link: facebook/metro#789

Reviewed By: rh389

Differential Revision: D34819044

Pulled By: motiz88

fbshipit-source-id: fe27b2c2af0281d5309d4c16f95762d4d4935eba
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants