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

separate keep_classnames & keep_fnames #2418

Closed
alexlamsl opened this issue Oct 30, 2017 · 12 comments
Closed

separate keep_classnames & keep_fnames #2418

alexlamsl opened this issue Oct 30, 2017 · 12 comments

Comments

@alexlamsl
Copy link
Collaborator

Bug report or feature request?

Feature

ES5 or ES6+ input?

ES6

Uglify version (uglifyjs -V)

uglify-es 3.1.6

This continues from #2416 (comment)

Basically, the proposal is to separate class-related functionalities from compress.keep_fnames into a new keep_classnames, then combine with that of mangle.keep_classnames to have a top-level --keep-classnames flag.

/cc @kzc

@kzc
Copy link
Contributor

kzc commented Oct 30, 2017

Your summary is good. I'd just add the ability to specify specific names via either RegExp/array for each mangle/compress keep*name option. Such a feature has been requested a few times in the past.

@alexlamsl
Copy link
Collaborator Author

The keep_fnames:/RawR/ part can be done separately on master, for those who are interested 😉

@kzc
Copy link
Contributor

kzc commented Oct 30, 2017

I could write it, but your version would be half the number of lines.

Also there would be some interaction and/or conflict with mangle reserved which I'm not too familiar with.

@alexlamsl
Copy link
Collaborator Author

Slightly OT: do you remember which issues call for keep_fnames:/patten/?

@kzc
Copy link
Contributor

kzc commented Oct 30, 2017

#1336

@kzc
Copy link
Contributor

kzc commented Oct 30, 2017

I propose grandfathered option behavior for harmony: when compress/mangle keep_classnames is not specified then keep_fnames will cover both class expression names and function expression names. This way old harmony compress --keep-fnames will work as before at the expense of retaining a few extra class expression names in mangle. This also has the advantage of not needing to specify keep_classnames at all in uglify-es - unless you want class name versus function name granularity. When ES6 code is transpiled to ES5 via babel or other means, classes become functions so there's class/function duality so it'd be nice to continue to use keep_fnames in either uglify-js or uglify-es without thinking about it.

@alexlamsl
Copy link
Collaborator Author

OTOH, keep_fnames never touches class names on master, so right now it actually behaves different between master and harmony.

Keeping them separate without any inter-dependency logic makes for cleaner code - yes it would change the current behaviour on harmony, but it's still pretty much alpha to begin with.

@kzc
Copy link
Contributor

kzc commented Oct 31, 2017

It would amount to just one extra if statement in minify to support the grandfathering heuristic outlined above in this untested partial patch:

--- a/lib/minify.js
+++ b/lib/minify.js
@@ -51,6 +51,7 @@ function minify(files, options) {
             compress: {},
             ecma: undefined,
             ie8: false,
+            keep_classnames: false,
             keep_fnames: false,
             mangle: {},
             nameCache: null,
@@ -62,12 +63,16 @@ function minify(files, options) {
             warnings: false,
             wrap: false,
         }, true);
+        if (!options.keep_classnames && options.keep_fnames) {
+            options.keep_classnames = options.keep_fnames;
+        }
         var timings = options.timings && {
             start: Date.now()
         };
         set_shorthand("ecma", options, [ "parse", "compress", "output" ]);
         set_shorthand("ie8", options, [ "compress", "mangle", "output" ]);
         set_shorthand("keep_fnames", options, [ "compress", "mangle" ]);
+        set_shorthand("keep_classnames", options, [ "compress", "mangle" ]);
         set_shorthand("toplevel", options, [ "compress", "mangle" ]);
         set_shorthand("warnings", options, [ "compress" ]);
         var quoted_props;

Even ignoring uglify-es options backwards compatibility, that usage just makes more sense. One could solely use --keep-fnames for the majority of mangle/compress scenarios without regard to whether they are dealing with function names or class names. In the odd case where they want to be specific about class names they could use --keep-classnames.

@alexlamsl
Copy link
Collaborator Author

Ah I see that you are confining that magic in top-level options, i.e. lib/minify.js instead of lib/compress.js or lib/scope.js - yeah I'm good with that.

@kzc
Copy link
Contributor

kzc commented Oct 31, 2017

Correct.

@kzc
Copy link
Contributor

kzc commented Nov 3, 2017

OT - uglify-es daily downloads now at 74K on a weekday. Up due to the release of uglifyjs-webpack-plugin@1.x and Angular 5 which uses that plugin.

Angular 5 transpiles down to ES5 so it would not exercise ES6 minification, however probably half of uglify-es users are minifying ES6+.

webpack@3 still ships with an older version of the uglify plugin that uses uglify-js@2.

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this issue Nov 24, 2017
alexlamsl added a commit that referenced this issue Nov 25, 2017
@yvbeek
Copy link

yvbeek commented Nov 27, 2018

keep_classnames doesn't seem to be documented?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants