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 terser to v5, support async minifiers #606

Closed
wants to merge 1 commit into from

Conversation

janicduplessis
Copy link
Contributor

Summary

Update terser to the latest version that supports more modern js syntax like optional chaining. The main breaking change in v5 is that minify is now async. To support this I also needed to add async minifier support in metro-transform-worker.

Test plan

  • Run tests
  • Test building my app bundle with minifier terser and untransformed optional chaining

@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 Nov 7, 2020
@@ -50,11 +50,7 @@ function minify({
/* $FlowFixMe(>=0.111.0 site=react_native_fb) This comment suppresses an
* error found when Flow v0.111 was deployed. To see the error, delete this
* comment and run Flow. */
const result = terser.minify(code, options);

if (result.error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Terser will now throw an error so there is no need for additional handling anymore.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Thank you 👍

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@cpojer
Copy link
Contributor

cpojer commented Nov 11, 2020

We have a big integration with Metro and Buck internally that relies on this code being sync and changing every Flow to be async is a ton of work I unfortunately won't be able to get to.

The good news is that with Hermes we shouldn't need a minifier, so we may be able to clean up our internal integration soon.

@janicduplessis
Copy link
Contributor Author

janicduplessis commented Nov 11, 2020

I see, so you are using the metro-minify-terser package directly? What about landing the changes in metro-transform-worker only so it supports async minifiers? This would make it easy then to just publish a terser-v5 minify plugin.

@cpojer
Copy link
Contributor

cpojer commented Nov 11, 2020

No, we don't actually use terser but we use the API synchronously in metro-transform-worker pretty heavily deep down a stack where changing everything to be async would be quite a bit of work. As said, with Hermes we may not need to support minifiers for much longer so maybe we can clean that up internally and then merge this.

@janicduplessis
Copy link
Contributor Author

Ah ok, the function in transform-worker is already async, but I guess it works since there are no await in it currently. Alright in this case let's just keep this open and we can revisit later when the internal infra changes.

@rockwotj
Copy link
Contributor

rockwotj commented Jan 4, 2022

Is there any hope to merging this? Our app is using terser and we've ran into a couple of terser bugs that it impossible to minify our app

@robhogan
Copy link
Contributor

FYI, I’ve rebased this internally now that the groundwork @cpojer mentioned has been done behind the scenes of #754, hope to land it shortly.

Metro Pull Requests automation moved this from Needs Triage to Closed Jan 20, 2022
nevilm-lt pushed a commit to nevilm-lt/metro that referenced this pull request Mar 14, 2022
Summary:
**Summary**

Update terser to the latest version that supports more modern js syntax like optional chaining. The main breaking change in v5 is that minify is now async. To support this I also needed to add async minifier support in metro-transform-worker.

**Test plan**

- Run tests
- Test building my app bundle with minifier terser and untransformed optional chaining

Pull Request resolved: facebook#606

Reviewed By: feedthejim

Differential Revision: D24883529

Pulled By: rh389

fbshipit-source-id: c9ed893535337ecc1ce16e433e3d689c2037be0a
nevilm-lt pushed a commit to nevilm-lt/metro that referenced this pull request Apr 21, 2022
Summary:
**Summary**

Update terser to the latest version that supports more modern js syntax like optional chaining. The main breaking change in v5 is that minify is now async. To support this I also needed to add async minifier support in metro-transform-worker.

**Test plan**

- Run tests
- Test building my app bundle with minifier terser and untransformed optional chaining

Pull Request resolved: facebook#606

Reviewed By: feedthejim

Differential Revision: D24883529

Pulled By: rh389

fbshipit-source-id: c9ed893535337ecc1ce16e433e3d689c2037be0a
nevilm-lt pushed a commit to nevilm-lt/metro that referenced this pull request Apr 22, 2022
Summary:
**Summary**

Update terser to the latest version that supports more modern js syntax like optional chaining. The main breaking change in v5 is that minify is now async. To support this I also needed to add async minifier support in metro-transform-worker.

**Test plan**

- Run tests
- Test building my app bundle with minifier terser and untransformed optional chaining

Pull Request resolved: facebook#606

Reviewed By: feedthejim

Differential Revision: D24883529

Pulled By: rh389

fbshipit-source-id: c9ed893535337ecc1ce16e433e3d689c2037be0a
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.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants