Uglify terser #243

haggholm · 2018-09-12T23:22:48Z

Summary

Add support for using terser. terser is a fork of uglify-es, which is not really maintained; and maintains API compatibility with uglify-es.

In order to avoid duplicating configuration like default options in metro-minify-uglify, this PR adds a parameter to metro-minify-uglify that allows passing in an alternative minifier module, and the new package metro-minify-terser calls this, passing in terser.

This is based on discussion in #230.

Test plan

The tests are simply copied from metro-minify-uglify, since the API is and the results should be identical.

Salakar · 2018-09-14T00:53:43Z

packages/metro-minify-terser/package.json

+  },
+  "license": "MIT",
+  "dependencies": {
+    "metro-minify-uglify": "^0.45.2",


Version needs to be hard set - e.g. remove the "^" - node CI tests should be good after this. 👍

Salakar · 2018-09-14T00:56:38Z

packages/metro-minify-terser/src/types.js.flow

+  inputMap?: ?BabelSourceMap,
+  filename: string,
+  options?: MinifyOptions,
+) => ResultWithMap;


Missing terser param type definition as 5th arg.

Salakar · 2018-09-14T00:57:24Z

packages/metro-minify-terser/src/types.js.flow

+type MinifierWithoutSourceMap = (
+  code: string,
+  options?: MinifyOptions,
+) => ResultWithoutMap;


Missing terser param type definition as 3rd arg.

Salakar · 2018-09-14T01:10:05Z

packages/metro-minify-uglify/src/minifier.js

@@ -23,17 +23,19 @@ import type {BabelSourceMap} from '@babel/core';
 function noSourceMap(
  code: string,
  options?: MinifyOptions = {},
+  uglifyModule: ?Object,
 ): ResultWithoutMap {


Types need updating here to add the extra uglifyModule arg.

Salakar · 2018-09-14T01:10:20Z

packages/metro-minify-uglify/src/minifier.js

 }

 function withSourceMap(
  code: string,
  sourceMap: ?BabelSourceMap,
  filename: string,
  options?: MinifyOptions = {},
+  uglifyModule: ?Object,


Types need updating here to add the extra uglifyModule arg.

CompuIves · 2018-09-14T13:24:41Z

This is great @haggholm! Thank you, Terser is a great minifier to have available!

I believe that we already have a configuration option to give a custom minifier to metro (https://github.com/facebook/metro/blob/master/packages/metro-config/src/defaults/index.js#L79). I think it would make most sense to add metro-minify-terser as package and give people the option to define it in that config instead of adding an extra parameter to the existing uglify package.

Does that make sense? I can also take a stab at this if that's okay with you!

haggholm · 2018-09-14T17:37:01Z

The reason I made it a parameter is that the alternative is to literally duplicate the code of metro-minify-uglify—certainly I could easily do this: it would literally involve just changing /uglify(-es)?/ with terser. but it seems to me like an unnecessary maintenance burden, as any changes to preferred options would have to be copied between the terser and uglify versions. The parameter to -uglify is not exposed, but just an internal thing to allow for shared implementation. Keep in mind that terser is API compatible with uglify-es and uglify-js@3, so the calling code in Metro will be identical.

I suppose an arguably cleaner way would be to have some sort of metro-minify-base (never directly referenced by users) which is exposed via -uglify and -terser versions.

I don’t really care too much ;) – after all I’m not a maintainer! I just wanted to keep it DRY. But I’ll happily do either one: (a) the way I first did it, (b) making -terser a straight copy of -uglify (with the underlying module substituted), or (c) creating an ‘internal’ base module that is exposed via specialised versions.

A fourth alternative (d) might be to add a different option: keep just metro-minify-uglify but allow an override of the uglify implementation used. Though this might just look like unnecessary proliferation of options.

But either way, let me know which way you’d like it to go and I’ll gladly contribute it. (Or, obviously, if you’d rather do it, that’s fine—I just want to use terser!)

rafeca · 2018-09-15T15:31:44Z

Hi @haggholm ! Thanks for the contribution! We're indeed exploring using terser ourselves, so this PR is really appreciated 😃

As @CompuIves mentioned, it would be better to make metro-minify-terser a first-class citizen and not make it go through metro-minify-uglify.

I agree that this means some duplication currently, but we are also going to simplify the API of the minifier so it'll only need to expsoe a single minify method (since the noSourceMap method is not used anywhere by Metro).

Also, we're going to have to move the flow types of the minifier API to the Metro repo, so they can be reused by any other potential minify implementation.

For now, I'm fine if you just copy the current metro-minify-uglify to create the metro-minify-terser package, and we'll do the cleaning later.

If you also want to do the cleaning, this is the two things that I would do (I'd appreciate a lot 😍):

1- Simplify the minifier API by getting rid of the noSourceMap method and renaming withSourceMap to minify.
2- Move the flow types of the minifier to the metro repository, so any package can import them easily.

Thanks again!

haggholm · 2018-09-17T18:57:01Z

Since the changes from the current master branch are really quite different, I made the changes in a new PR to supercede this one: #256

rafeca · 2018-09-18T14:16:01Z

Closing this PR then

Summary:  **Summary**  Add support for using [terser](https://github.com/fabiosantoscode/terser/). `terser` is a fork of `uglify-es`, which is not really maintained; and maintains API compatibility with `uglify-es`. In order to avoid duplicating configuration like default options in `metro-minify-uglify`, this PR adds a parameter to `metro-minify-uglify` that allows passing in an alternative minifier module, and the new package `metro-minify-terser` calls this, passing in `terser`. This is based on discussion in #230. After earlier submitting #243, I created this alternative PR to address comments from rafeca **Test plan**  The tests are simply copied from `metro-minify-uglify`, since the API is and the results should be identical. Pull Request resolved: #256 Reviewed By: pvdz Differential Revision: D9908604 Pulled By: rafeca fbshipit-source-id: 30dfe4f8cc6b614e50874be51aad0883a1997061

haggholm added 3 commits September 12, 2018 16:03

Add metro-minify-terser

9467582

Bugfix

a67fdfb

Remove accidentally committed change

a0c2e76

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 Sep 12, 2018

Salakar reviewed Sep 14, 2018

View reviewed changes

haggholm mentioned this pull request Sep 17, 2018

Uglify terser copy #256

Closed

rafeca closed this Sep 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uglify terser #243

Uglify terser #243

haggholm commented Sep 12, 2018

Salakar Sep 14, 2018

Salakar Sep 14, 2018

Salakar Sep 14, 2018

Salakar Sep 14, 2018

Salakar Sep 14, 2018

CompuIves commented Sep 14, 2018

haggholm commented Sep 14, 2018

rafeca commented Sep 15, 2018

haggholm commented Sep 17, 2018

rafeca commented Sep 18, 2018

Uglify terser #243

Uglify terser #243

Conversation

haggholm commented Sep 12, 2018

Salakar Sep 14, 2018

Choose a reason for hiding this comment

Salakar Sep 14, 2018

Choose a reason for hiding this comment

Salakar Sep 14, 2018

Choose a reason for hiding this comment

Salakar Sep 14, 2018

Choose a reason for hiding this comment

Salakar Sep 14, 2018

Choose a reason for hiding this comment

CompuIves commented Sep 14, 2018

haggholm commented Sep 14, 2018

rafeca commented Sep 15, 2018

haggholm commented Sep 17, 2018

rafeca commented Sep 18, 2018