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

properties are not mangled if it's in a deferred top level object #397

Closed
gdh1995 opened this issue Jul 9, 2019 · 3 comments · Fixed by #424
Closed

properties are not mangled if it's in a deferred top level object #397

gdh1995 opened this issue Jul 9, 2019 · 3 comments · Fixed by #424

Comments

@gdh1995
Copy link
Contributor

gdh1995 commented Jul 9, 2019

Bug report or Feature request? Bug report

Version (complete output of terser -V or specific git commit) from v3.14.1 to the latest v4.1.2

code to run

#!/usr/bin/env node
"use strict";
var opts = {
  "mangle": {
    "properties": true,
    "toplevel": true
  },
  nameCache: { vars: {}, props: {} }
};

var terser = require('terser');
terser.minify('var Bar = {};', opts);
var result = terser.minify('var Foo = { foo: function() { return Bar.bar() } };', opts);
var expected = "var r={o:function(){return a.v()}};";
console.log("[task] test uglifying external properties",
    "\n[Current ]", result.code, "\n[Expected]", expected);

expected result

var r={o:function(){return a.v()}};

real result

var r={o:function(){return a.bar()}};

why this is a bug

In a real case, if a Bar is defined in some later files, I want to pre-define Bar to make terser uglifiy this name, and I also want to uglify all its properties. But I don't like to pass all input files to terser during one calling to .minify

  • I'm using Gulp, and the file which defines Bar is in another task
  • currently, I add an extra step to call terser.minify("var Bar = {};") by myself, and abort its output
    • so that the gulp task's output keeps smallest.

This usage works well under terser v3.10.*, and something becames wrong since terser v3.14.1 .

I've read #219 , but I think my usage is still worth a new if-branch.

debugging

I find a commit in v3.14.1 seems to blame:

https://github.com/terser-js/terser/blob/b4dda6d2ccdc94ac42f6dbf459af50ac343f9454/lib/propmangle.js#L168-L173

When I reverted it to a simple add(node.property);, this bug disappeared.

gdh1995 added a commit to gdh1995/vimium-c that referenced this issue Jul 9, 2019
@gdh1995 gdh1995 changed the title [bug] some properteis are not mangled in some cases [bug] some properteis are not mangled if it's in a top level object which is also mangled Jul 9, 2019
@gdh1995 gdh1995 changed the title [bug] some properteis are not mangled if it's in a top level object which is also mangled some properteis are not mangled if it's in a deferred top level object Jul 9, 2019
@gdh1995
Copy link
Contributor Author

gdh1995 commented Jul 9, 2019

The old issue message is here: it's too long and somehow incorrect, so I move it here just to archive it.

description

When I compiled Vimium C at commit gdh1995/vimium-c@c7cb108 using different versions of terser, the terser v4.1.2 cause null pointer exception, and after debugging, I found some properties are not mangled unexpectedly. After further tests, I've confirmed:

  • all versions since v3.14.1, including v3.16.0, v3.16.1, v3.17.0, v4.1.2, have such a bug
  • while v3.14.0, v3.13.1, v3.11.0, v3.10.12 and v3.10.11 works well (at least only caring about the function for mangling properties)

I tested removing it in v3.14.1 but this bug still exists.

error details

Vimium C has quite a few steps to compile it:

  1. firstly, compile all TypeScript scripts to JavaScript and save them in a folder of dist/.build/
  • the gulp task is gulp build/ts
  1. the next step is to uglify files in the dist/.build/lib and dist/.build/content folder
  • the gulp task which uses terser is named "min/content"
  • in this step, terser generates good results (at least all wanted properties are mangled)
  1. the third step is to uglify files in dist/.build/background, and some other steps are for dist/.build/pages and dist/.build/front
  • the gulp tasks are min/bg and min/others
  • here, all config data passed to terser is just the same as the task min/content in the step 2,
    except that I load a "nameCache" dict and setup config.nameCache to it,
    • the cache looks like { vars: {...}, props: {...}, timestamp: 0 } - vars and props are from terser's output in step 2, and they will be re-wrapped and passed to terser.

And:

  • I removed the extra nameCache by running ENABLE_NAME_CACHE=0 DEBUG=1 gulp min/bg, this bug also occurred.
  • I tried modifing Gulpfile.js and runs min/bg ignoring min/content, but it was still there.

@gdh1995 gdh1995 changed the title some properteis are not mangled if it's in a deferred top level object properties are not mangled if it's in a deferred top level object Jul 9, 2019
@fabiosantoscode
Copy link
Collaborator

Hey there!

Nice extension :)

What you described is not exactly a bug. Terser has no way of knowing whether lowercase bar is a property that exists.

What I found though, is that if I create another artificial file and use Bar.bar in any way, Terser does not "find out" that bar is a property, so it doesn't minify .bar in the third code example. Like this:

#!/usr/bin/env node                                                                           
"use strict";                                                                                 
var opts = {                                                                                  
  "mangle": {                                                                                 
    "properties": true,                                                                       
    "toplevel": true                                                                          
  },                                                                                          
  nameCache: { vars: {}, props: {} }                                                          
};                                                                                            
                                                                                              
var terser = require('./dist/bundle.js');                                                     
terser.minify('var Bar = {};', opts);                                                         
terser.minify('Bar.bar = 123', opts);  // <- this is what I added                                                         
console.log(opts.nameCache)                                                                   
var result = terser.minify('var Foo = { foo: function() { return Bar.bar() } };', opts);      
var expected = "var r={o:function(){return a.v()}};";                                         
console.log("[task] test uglifying external properties",                                      
    "\n[Current ]", result.code, "\n[Expected]", expected);                                   

I suppose this is the bug you're trying to report. In that case, sounds like an interesting bug. And indeed I want this to be a Terser feature. When this is fixed I'll ensure there's a test that protects your use case.

Thanks for your report!

@gdh1995
Copy link
Contributor Author

gdh1995 commented Jul 17, 2019

Thanks.

idea 1

If it's hard to tell whether a variable is from outsides, I think a new option may be suitable to leave this problem to final users.

idea 2

For the project I mentioned above, it's in TypeScript and I've applied many "strict" rules, so I have a terser config looking like this (from https://github.com/gdh1995/vimium-c/blob/master/scripts/uglifyjs.dist.json):

var config = {
  "compress": {
    "properties": true,
    "pure_getters": true,
    "toplevel": false,
  },
  "mangle": {
    "properties": {
      "regex": /^_|_$/
    },
    "toplevel": true,
    "reserved": [
      // # expected names in web-extension content
      "WeakSet", "Set",
      // # expected names in 3rd-party extensions' contents
      "requestIdleCallback",
      // # content global names:
      "browser",
      "VimiumInjector", "VDom", "VKey",
      "VHints", "VScroller", "VOmni", "VFind", "VVisual", "VMarks",
      "VHud", "VPort", "VEvent",
      // ...
    ],
    // I've ensured all other global names of my code MUST start / end with "_",
    // and they are mangled correctly by terser v3.10.*.
  }
};

And as said in the comment, only those global variables name which starts/ends with "_" will be mangled, so I wonder if terser may think "all matched global names and their properties visits should be uglified" when it knows:

  • "not all" properties are expected to be mangled
  • or, some global variables have been mangled, and pure_getters is true.

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

Successfully merging a pull request may close this issue.

2 participants