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

Module scope #3007

Closed
wants to merge 8 commits into from
Closed

Module scope #3007

wants to merge 8 commits into from

Conversation

fabiosantoscode
Copy link
Contributor

Taking a stab at #2964.

This creates a --module option, that's passed to compress and mangle both, where in compress, strict mode is turned on, and in mangle, it creates a new module-level scope for the file.

This means that top-level symbols, when you use --module, get mangled!

lib/compress.js Outdated
@@ -138,6 +139,9 @@ function Compressor(options, false_by_default) {
funcs: toplevel,
vars: toplevel
};
if (this.options['module']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

double quotes please

lib/compress.js Outdated
@@ -138,6 +139,9 @@ function Compressor(options, false_by_default) {
funcs: toplevel,
vars: toplevel
};
if (this.options['module']) {
this.directives['use strict'] = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

If compress module is enabled, then compress toplevel can also be enabled.

lib/scope.js Outdated
if (options.module) {
scope = new AST_Scope(self);
}
self.parent_scope = scope;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than creating a new scope all that needs to be done is to set mangle toplevel to true when mangle module is enabled. Then it will be handled automatically.

expect: {
let e = 10;
}
}
Copy link
Contributor

@kzc kzc Mar 15, 2018

Choose a reason for hiding this comment

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

Once the toplevel changes have been made a new test could be added:

module_enabled: {
    options = {
        module: true,
        reduce_vars: true,
        unused: true,
    }
    mangle = {
        module: true,
    }
    input: {
        let apple = 10, b = 20;
        console.log(apple++, b, apple++);
        export { apple };
    }
    expect: {
        let o = 10;
        console.log(o++, 20, o++);
        export { o as apple };
    }
}

Notice how b can be dropped due to compress toplevel being true.

@kzc
Copy link
Contributor

kzc commented Mar 15, 2018

I suggest using this approach for lib/compress.js and lib/scope.js:

diff --git a/lib/compress.js b/lib/compress.js
index 886a4b5..2654a07 100644
--- a/lib/compress.js
+++ b/lib/compress.js
@@ -73,6 +73,7 @@ function Compressor(options, false_by_default) {
         keep_fnames   : false,
         keep_infinity : false,
         loops         : !false_by_default,
+        module        : false,
         negate_iife   : !false_by_default,
         passes        : 1,
         properties    : !false_by_default,
@@ -130,6 +131,10 @@ function Compressor(options, false_by_default) {
             return top_retain.indexOf(def.name) >= 0;
         };
     }
+    if (this.options["module"]) {
+        this.directives["use strict"] = true;
+        this.options["toplevel"] = true;
+    }
     var toplevel = this.options["toplevel"];
     this.toplevel = typeof toplevel == "string" ? {
         funcs: /funcs/.test(toplevel),
diff --git a/lib/scope.js b/lib/scope.js
index 09687aa..c94fe89 100644
--- a/lib/scope.js
+++ b/lib/scope.js
@@ -514,9 +514,13 @@ AST_Toplevel.DEFMETHOD("_default_mangler_options", function(options) {
         ie8         : false,
         keep_classnames: false,
         keep_fnames : false,
+        module      : false,
         reserved    : [],
         toplevel    : false,
     });
+    if (options.module) {
+        options.toplevel = true;
+    }
     if (!Array.isArray(options.reserved)) options.reserved = [];
     // Never mangle arguments
     push_uniq(options.reserved, "arguments");

@kzc
Copy link
Contributor

kzc commented Mar 15, 2018

The CLI option has an issue:

$ echo 'let foo = 1, bar = 2; export { foo };' | bin/uglifyjs --module -cm
Supported options:
  cache            null
  eval             false
  ie8              false
  keep_classnames  false
  keep_fnames      false
  properties       false
  reserved         []
  safari10         false
  toplevel         false
ERROR: `module` is not a supported option
    at DefaultsError.get

This patch ought to fix it:

--- a/lib/minify.js
+++ b/lib/minify.js
@@ -52,6 +52,7 @@ function minify(files, options) {
             keep_classnames: undefined,
             keep_fnames: false,
             mangle: {},
+            module: false,
             nameCache: null,
             output: {},
             parse: {},
@@ -76,6 +77,7 @@ function minify(files, options) {
         set_shorthand("ie8", options, [ "compress", "mangle", "output" ]);
         set_shorthand("keep_classnames", options, [ "compress", "mangle" ]);
         set_shorthand("keep_fnames", options, [ "compress", "mangle" ]);
+        set_shorthand("module", options, [ "compress", "mangle" ]);
         set_shorthand("safari10", options, [ "mangle", "output" ]);
         set_shorthand("toplevel", options, [ "compress", "mangle" ]);
         set_shorthand("warnings", options, [ "compress" ]);
@@ -87,6 +89,7 @@ function minify(files, options) {
                 ie8: false,
                 keep_classnames: false,
                 keep_fnames: false,
+                module: false,
                 properties: false,
                 reserved: [],
                 safari10: false,
$ echo 'let foo = 1, bar = 2; export { foo };' | bin/uglifyjs --module -cm
let e=1;export{e as foo};

Would be nice if there was a CLI test for --module in test/mocha/cli.js.

@fabiosantoscode
Copy link
Contributor Author

Ok, I think this is all :)

README.md Outdated
@@ -110,6 +110,7 @@ a double dash to prevent input files being used as option arguments:
--keep-classnames Do not mangle/drop class names.
--keep-fnames Do not mangle/drop function names. Useful for
code relying on Function.prototype.name.
--module Input is an ES6 module
Copy link
Contributor

@kzc kzc Mar 16, 2018

Choose a reason for hiding this comment

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

+    --module                    Input is an ES6 module. If `compress` or `mangle` is
+                                enabled then the `toplevel` option will be enabled.

README.md Outdated
@@ -496,6 +497,9 @@ if (result.error) throw result.error;
- `mangle.properties` (default `false`) — a subcategory of the mangle option.
Pass an object to specify custom [mangle property options](#mangle-properties-options).

- `module` (default `false`) — Use when minifying an ES6 module. "use strict"
is implied and names can be mangled on the top scope.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add:

If `compress` or `mangle` is enabled then the `toplevel` option will be enabled.

@fabiosantoscode
Copy link
Contributor Author

Done too!

@kzc
Copy link
Contributor

kzc commented Mar 18, 2018

lgtm

@kzc
Copy link
Contributor

kzc commented Mar 19, 2018

We forgot about dealing with the implicit "use strict" for use with modules in lib/parse.js...

$ echo 'with (console) log("\101");' | node_modules/.bin/acorn --module
'with' in strict mode (1:0)
$ echo 'with (console) log("\101");' | bin/uglifyjs --module
with(console)log("A");

I think this should handle it:

diff --git a/lib/minify.js b/lib/minify.js
index ee1d478..e360da0 100644
--- a/lib/minify.js
+++ b/lib/minify.js
@@ -52,6 +52,7 @@ function minify(files, options) {
             keep_classnames: undefined,
             keep_fnames: false,
             mangle: {},
+            module: false,
             nameCache: null,
             output: {},
             parse: {},
@@ -76,6 +77,7 @@ function minify(files, options) {
         set_shorthand("ie8", options, [ "compress", "mangle", "output" ]);
         set_shorthand("keep_classnames", options, [ "compress", "mangle" ]);
         set_shorthand("keep_fnames", options, [ "compress", "mangle" ]);
+        set_shorthand("module", options, [ "parse", "compress", "mangle" ]);
         set_shorthand("safari10", options, [ "mangle", "output" ]);
         set_shorthand("toplevel", options, [ "compress", "mangle" ]);
         set_shorthand("warnings", options, [ "compress" ]);
@@ -87,6 +89,7 @@ function minify(files, options) {
                 ie8: false,
                 keep_classnames: false,
                 keep_fnames: false,
+                module: false,
                 properties: false,
                 reserved: [],
                 safari10: false,
diff --git a/lib/parse.js b/lib/parse.js
index d8cc9ac..bd2434a 100644
--- a/lib/parse.js
+++ b/lib/parse.js
@@ -859,6 +859,7 @@ function parse($TEXT, options) {
         expression     : false,
         filename       : null,
         html5_comments : true,
+        module         : false,
         shebang        : true,
         strict         : false,
         toplevel       : null,
@@ -2913,6 +2914,7 @@ function parse($TEXT, options) {
         var start = S.token;
         var body = [];
         S.input.push_directives_stack();
+        if (options.module) S.input.add_directive("use strict");
         while (!is("eof"))
             body.push(statement());
         S.input.pop_directives_stack();
$ echo 'with (console) log("\101");' | bin/uglifyjs
with(console)log("A");
$ echo 'with (console) log("\101");' | bin/uglifyjs --module
Parse error at 0:1,0
with (console) log("\101");
^
ERROR: Strict mode may not include a with statement

Would also need an entry in the README parse options section, and ideally a mocha test to prove it works.

@kzc
Copy link
Contributor

kzc commented Mar 19, 2018

This is obscure, but to get the parse option acorn to work with ES6 modules the following patch is required:

--- a/bin/uglifyjs
+++ b/bin/uglifyjs
@@ -44,6 +44,7 @@ program.option("--ecma <version>", "Specify ECMAScript release: 5, 6, 7 or 8.");
 program.option("--ie8", "Support non-standard Internet Explorer 8.");
 program.option("--keep-classnames", "Do not mangle/drop class names.");
 program.option("--keep-fnames", "Do not mangle/drop function names. Useful for code relying on Function.prototype.name.");
+program.option("--module", "Input is an ES6 module");
 program.option("--name-cache <file>", "File to hold mangled name mappings.");
 program.option("--rename", "Force symbol expansion.");
 program.option("--no-rename", "Disable symbol expansion.");
@@ -66,6 +67,7 @@ if (!program.output && program.sourceMap && program.sourceMap.url != "inline") {
     "compress",
     "ie8",
     "mangle",
+    "module",
     "safari10",
     "sourceMap",
     "toplevel",
@@ -195,7 +197,8 @@ function run() {
                     return require("acorn").parse(files[name], {
                         locations: true,
                         program: toplevel,
-                        sourceFile: name
+                        sourceFile: name,
+                        sourceType: options.module || program.parse.module ? "module" : "script",
                     });
                 });
             } else if (program.parse.spidermonkey) {

With patch:

$ echo 'with (console) log("\101");' | bin/uglifyjs -p acorn
with(console)log("A");
$ echo 'with (console) log("\101");' | bin/uglifyjs -p acorn --module
ERROR: 'with' in strict mode (1:0)
$ echo 'with (console) log("\101");' | bin/uglifyjs -p acorn,module
ERROR: 'with' in strict mode (1:0)

Note: The utility of this particular patch is limited because uglify-es only supports ES5 for the acorn parse option at this time. See #968.

@fabiosantoscode
Copy link
Contributor Author

Well thought @kzc, done!

@guybedford
Copy link

Great to see this happening!

@fabiosantoscode
Copy link
Contributor Author

Merged under my repo

@guybedford
Copy link

@fabiosantoscode have you tried asking @alexlamsl for publish rights to the uglify-es package, as it seems like you're effectively offering to maintain this?

@fabiosantoscode
Copy link
Contributor Author

I think it has been refused, IIRC.

@guybedford
Copy link

@fabiosantoscode could you enable issues on your forked repo and we can discuss further there?

@fabiosantoscode
Copy link
Contributor Author

@guybedford well spotted! Issues enabled

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

Successfully merging this pull request may close these issues.

None yet

3 participants