Uglifyjs: [ES6] `-m toplevel` should not mangle names in `export default`

[kzc]

export default names should not be mangled if -m toplevel is in effect:

$ echo 'import Base from "Base.js"; export default class Derived extends Base {}' | bin/uglifyjs -m toplevel
import s from"Base.js";export default class a extends s{};

$ echo 'export default function Foo(){}' | bin/uglifyjs -m toplevel
export default function n(){};



md5-92b5344d397b861823844bc303ba276f



$ echo 'import Base from "Base.js"; export class Derived extends Base {}' | bin/uglifyjs -m toplevel
import e from"Base.js";export class Derived extends e{};



md5-3d61a4e2c92946e488dbbd3fd91c8831



$ echo 'export function Foo(){}' | bin/uglifyjs -m toplevel
export function Foo(){};

[alexlamsl]

Mangle symbols within export default seems to be deliberate:
https://github.com/mishoo/UglifyJS2/blob/11923e3ae8a6917f5ec923e5d57e943f8aee1871/lib/scope.js#L238
https://github.com/mishoo/UglifyJS2/blob/11923e3ae8a6917f5ec923e5d57e943f8aee1871/lib/scope.js#L66

[alexlamsl]

In fact, there is a test explicitly added to guard this behaviour:
https://github.com/mishoo/UglifyJS2/commit/39d4d7e20ae013b1c6071964ce048ee86cbff6e9#diff-9f211bedf6a1b83d320703c4bc9e72baR124

[kzc]

If it was deliberate, it was a mistake - even if I may have been involved with the decision.

[alexlamsl]

@kzc so that we are on the same page - export default is no different from export from mangle's PoV?

[kzc]

I'm trying to uglify all the individual source files in rollup with

--toplevel -mc unsafe,pure_getters,passes=3

and #2129 and #2131 prevent that from happening. Also have to add -b to work around buble bugs, but that's an issue unrelated to uglify-es.

[kzc]

export default is no different from export from mangle's PoV?

I used to not think so, but based on my experience with https://github.com/mishoo/UglifyJS2/issues/2129#issuecomment-309940785 I now believe this to be true.

[alexlamsl]

About https://github.com/mishoo/UglifyJS2/issues/2129#issuecomment-309940785 - so you just do npm install rollup and then do node bin\uglifyjs <*.js> --toplevel -mc unsafe,pure_getters,passes=3 over each and every .js file in the folder?

[kzc]

I'm uglifying every file in rollup/src/ (and any subdirectories) using

--toplevel -mc unsafe,pure_getters,passes=3 -b

I apply this patch to rollup:

--- a/rollup.config.js
+++ b/rollup.config.js
@@ -24,7 +24,7 @@ export default {
                buble({
                        include: [ 'src/**', 'node_modules/acorn/**' ],
                        target: {
-                               node: '0.12'
+                               node: 6
                        }
                }),

then run npm test

[kzc]

I've encountered another problem:

$ echo 'export const { keys } = Object;' | bin/uglifyjs -m toplevel
export const{keys:c}=Object;

We need to suppress mangle of exported destructuring symbols as well.

[kzc]

@alexlamsl So far with your latests PRs I've successfully run npm test after replacing rollup/src/ files with uglifyjs -c toplevel,passes=3,pure_getters,unsafe -m -b equivalents. Top level mangle still does not work.

I also had to contend with an acorn ES6 bug that has been fixed but not yet released. See:
https://github.com/ternjs/acorn/issues/561
https://github.com/ternjs/acorn/commit/98b032652ffce0f53e5cb84435b4fc1bf819c1fc

[alexlamsl]

I need to go out for a meeting now, but do leave a note on whether latest commit works for toplevel.

Thanks a lot for the extensive testing as usual :wink:

[kzc]

Your export const { keys } = Object; fix does indeed work. But now have to figure out what's going on with another npm test error.

[kzc]

The next issue deals with this source file:

$ cat src/utils/fs.js 

import * as fs from "fs";

import { dirname as i } from "./path.js";

function n(t) {
    const c = i(t);
    try {
        fs.readdirSync(c);
    } catch (unused) {
        n(c), fs.mkdirSync(c);
    }
}

export * from "fs";

export function writeFile(i, t) {
    return new Promise((c, f) => {
        n(i);
        fs.writeFile(i, t, n => {
            n ? f(n) : c();
        });
    });
};

which uglifyjs src/utils/fs.js --toplevel -m -c -b converts to:

function n(t) {
    const c = i(t);
    try {
        fs.readdirSync(c);
    } catch (i) {
        n(c), fs.mkdirSync(c);
    }
}

import * as fs from "fs";

import { dirname as i } from "./path.js";

export * from "fs";

export function writeFile(i, t) {
    return new Promise((c, f) => {
        n(i);
        fs.writeFile(i, t, n => {
            n ? f(n) : c();
        });
    });
};

Notice how the function was hoisted above the import statements. Not sure if that's an issue, but I think it's fine.

Also notice the catch (i) generated by uglify:

function n(t) {
    const c = i(t);
    try {
        fs.readdirSync(c);
    } catch (i) {
        n(c), fs.mkdirSync(c);
    }

which rollup ultimately bundles up and errors out with:

dist/rollup.js:
---
65 : function n$1(t) {
66 :     const c = path.dirname(t);
67 :     try {
68 :         fs.readdirSync(c);
69 :     } catch (path.dirname) {
                      ^
Unexpected token (69:17)

The substitution of i with path.dirname in catch (i) made by rollup is incorrect.

If I change catch (i) to catch (unused) then re-run npm test it continues to hit a different error - a buble scope bug with a for-of loop. Which is where I gave up.

[kzc]

The remaining issues are unrelated to uglify-es. The latest ES6 toplevel PRs are good to go.

[alexlamsl]

So rollup also has something similar to reduce_vars - interesting.

Thanks for the investigation. I shall merge these, then may be it's time for 3.0.19