Could you please add the positive and negative test case scenarios I mentioned in #1198?

#1198 (comment)
#1198 (comment)
#1198 (comment)

What happens in a mixed LHS and RHS scenario?

        DEBUG = 1;
        DEBUG++;
        DEBUG += 1;
        f(DEBUG);
        x = DEBUG;

Will all DEBUG replacements be disallowed with an appropriate warning?

Or at the very least a fatal error thrown upon first attempt to change a LHS expression.

This PR breaks existing CLI usage:

uglify-js@2.7.5:

$ echo 'console.log(D);' | uglifyjs --define D=5 -cm
console.log(5);

this PR:

$ echo 'console.log(D);' | bin/uglifyjs --define D=5 -cm
console.log(D);

Could you please add a mocha test for the CLI --define option after this is fixed?

Also, how to set compress global_defines objects and arrays on the CLI?

Would prefer if existing test files are not renamed. That way it is easier to discern if the tests' behavior has been changed.

What happens in a mixed LHS and RHS scenario?

Unfortunately that would replace the RHS cases (and leave the LHS ones alone) - this is the same behaviour as before this PR though.

Investigating the command-line breakage at the moment, will add mocha and those other tests as you mentioned above. Then I'll ponder about the mixed case and see how to go about fixing it. (or may be open another PR?)

Unfortunately that would replace the RHS cases (and leave the LHS ones alone) - this is the same behaviour as before this PR though.

The current behavior is not great in this regard. Would prefer it to throw a fatal exception at the first attempt to set a LHS variable so the user is aware that their code must be fixed.

The current behavior is not great in this regard. Would prefer it to throw a fatal exception at the first attempt to set a LHS variable so the user is aware that their code must be fixed.

Understood - you want this to be in the same PR or separate?

You can make the fixes in this PR I think.

Can you please add a [WIP] prefix?

The current behavior is not great in this regard. Would prefer it to throw a fatal exception at the first attempt to set a LHS variable so the user is aware that their code must be fixed.

On second thought, a warning for LHS variable use would be better. Changing to it a fatal exception would be a breaking change unfortunately. There's probably use of mixed RHS/LHS use in the wild.

By the way it's possible to test for warnings in the test framework. See:

https://github.com/mishoo/UglifyJS2/blob/c55dd5ed74888d533cf8402c6ba3612916ba2885/test/compress/issue-1034.js#L29-L34

Incorporated those tests you've mentioned above except the first one, which is a mixed LHS/RHS case.

I shall now work on that mixed case and emit warnings as requested. 😉

Incorporated those tests you've mentioned above except the first one, which is a mixed LHS/RHS case.

I'd like to see a mixed use LHS/RHS test case with expect_warnings: to see what happens.

Edit: I should better read your comments...

I shall now work on that mixed case and emit warnings as requested

I'd like to see a mixed use LHS/RHS test case with expect_warnings: to see what happens.

Would you like something difference from https://github.com/mishoo/UglifyJS2/pull/1467/files#diff-a304d9611b5625800249b63bea77fce7R33 ?

FWIW, test/benchmark.js returns identical SHA1s on master and e54fd32

From a CLI usability point of view, this is awkward:

$ echo 'console.log(C.D)' | bin/uglifyjs -c unsafe --define C={D:true} 
console.log(!0);

The unsafe is not necessary, and the object nesting is not intuitive.

Users would expect to use it in this way:

$ echo 'console.log(C.D)' | bin/uglifyjs -c --define C.D=true 
console.log(C.D);

with it producing:

console.log(!0);

Think of the original use case of replacing process.env.NODE_ENV. Users would like to enter something like this:

$ echo 'console.log(process.env.NODE_ENV)' | bin/uglifyjs -c --define 'process.env.NODE_ENV="production"'

That is the use case I have in mind, though I use it via API. Your case about --define certainly makes sense - I'll look into making it happen.

unsafe is required due to #1425 - and I think we can relax that at a future release after some field testing.

unsafe is required due to #1425 - and I think we can relax that at a future release after some field testing.

I see. Could you document that in the README? It's not an obvious association.

Could you document that in the README?

Will do 👍

With regards to global_defs, do you agree specifying:

global_defs: {
  process: {
    env: { NODE_ENV: "production" }
  }
}

to be more natural in the API form?

I'm not sure. Webpack apparently supports both:

new webpack.DefinePlugin({
    'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV || 'development')
}),

and

new webpack.DefinePlugin({
  "process.env": {
    NODE_ENV: JSON.stringify("production")
  }
});

Can you do something similar - assume dots in keys imply parent objects?

@kzc feel free to suggest anything to expand on README.md - writing documentation is not my forte 😅

Your README addition looks fine.

With latest PR did not see any warnings for LHS defines:

$ echo 'console.log(++D.E);' | bin/uglifyjs --define D.E=123 -c unsafe
console.log(++123);

$ echo 'console.log(++D.E);' | bin/uglifyjs --define D=123 -c unsafe
console.log(++123..E);

Should be all fixed up now.

(Travis CI timed out.)

Just checked 84d3d5a - SHA1s from test/benchmark.js still identical with those from master.

Not directly relevant to this PR per se, but there are code out there which uses IIFEs to capture global variables:

(function(window) {
  // mian program code
})(window);

Which aids mangling, but will be opaque to global_defs for the purpose of conditional compilation.

Warning is incorrect here in latest PR:

$ echo 'console.log(++A.B.C);' | bin/uglifyjs --define A.B.X=123 -c unsafe
WARN: global_defs["A"] redefined [-:1,14]
console.log(++A.B.C);

Just curious how we might achieve replacing a symbol name with another symbol name...

String replacement "works" in a sense:

$ echo 'println(123);' | bin/uglifyjs --define println='"console.log"' -c
"console.log"(123);

but how can we achieve the following output?

console.log(123);

Sorry - let's try that again...

With this PR the following does not work:

$ echo 'println(123);' | bin/uglifyjs --define println=console.log -c
println(123);

$ echo 'console.log(456);' | bin/uglifyjs --define console.log=println -c
undefined:2
return (println)
        ^

ReferenceError: println is not defined

I recognize it does not work in uglify-js@2.7.5 either. But could it be supported somehow?

@kzc sorry for the botched fix - please try this second attempt after some sleep.

Meanwhile I'll look at the case above.

Swapping println with console.log is actually AST_Node substitutions. While the command-line case looks simple as you demonstrated above, the API case seems a little contrived:

{
  global_defs: {
    println: UglifyJS.parse("console.log").body[0].body
  }
}

Any better ideas?

Also, /me already thinks bin/uglifyjs --define A=B,B=A is a fun thing to try 😈

Speaking of the devil, echo a; | bin/uglifyjs -d A=console.log('PWNED!') -c gives some interesting results.

@kzc so I think about it some more on this AST_Node substitution, and it's essentially a TreeTransformer walk replacing all the matching AST_SymbolRef with a supplied AST_Node, skipping over all the isLHS() instances.

Would you mind elaborating on the general use case of this feature? I'm trying to gauge the scope of work to see if this is straightforward or I should branch it to another PR.

Another thing I'm thinking about for future work is to allow functions to global_defs:

{
  global_defs: {
    require: function(path) {
      if (path == "./config") return { env: "production", isNode: false }
      throw this;
    }
  }
}

Which will also work on command line as well.

I thought substituting functions would be a nice to have. I appreciate it's non-trivial and involves creating nodes. It's not essential.

Closing in favour of #1469.