https://github.com/mishoo/UglifyJS2/pull/1878/files?w=1 for less scary diff

output: "ast"

How would the following output scenarios be supported?

1. output AST, output code string
2. do not output AST, do not output code string
3. output AST, do not output code string
4. do not output AST, output code string

I'd prefer the output: { ast: true, code: false } syntax because the comparisons are faster at the output level - not relying on string comparisons or regexes, but it's up to you. As long as the 4 options above are supported. Keep in mind that outputting code strings is frequently called in compress.js to choose the shorter length code string.

How would the following output scenarios be supported?

I thought about supporting those, but take options.output.comments for instance - if we output both code and AST, they won't reflect each other because the code would have (some) comments dropped whereas the AST would still have them.

but take options.output.comments for instance

I can live with the AST/code mismatch in that scenario. Comments are a whole different problem unto themselves.

Keep in mind that outputting code strings is frequently called in compress.js to choose the shorter length code string.

The way this PR is done, output:"ast" completely bypasses OutputStream, so it won't affect AST_Node.print_to_string() which is used within lib/compress.js in way you described.

it won't affect AST_Node.print_to_string() which is used within lib/compress.js in way you described.

Okay, as long as it does not incur any output cost for AST_Node.print_to_string() I'm good with it. Just have to support the 4 scenarios in #1878 (comment)

I can live with the AST/code mismatch in that scenario. Comments are a whole different problem unto themselves.

In that case, we can just output both ast and code all the time. There isn't any extra CPU memory usage anyway.

we can just output both ast and code all the time. There isn't any extra CPU memory usage anyway.

No, no, no. That would mean that the AST cannot be released in most common minify() scenarios causing more memory use. We only want to return result.ast from minify() when requested.

Ah wait, what I meant to say is we don't need ast:true but we do need code:false, because if we only want to parse() then OutputStream can be bypassed completely without any runtime overhead.

That would mean that the AST cannot be released in most common minify() scenarios causing more memory use.

True, VMs do optimise memory usage quite aggressively. I'll update the PR with your proposal then.

I think the minify() output options { ast : boolean, code : boolean } would cover all bases in the simplest way. Obviously ast would default to false, and code would default to true.

@kzc agreed - PTAL at 170db9b

As of your latest PR commit, we no longer need to export _parse and _Compressor, correct?

PTAL at 170db9b

looks good

As of your latest PR commit, we no longer need to export _parse and _Compressor , correct?

After removing that test under test/mocha/minify.js, yes. I am planning to do so in #1877 after this PR merges.

If the CLI bin/uglifyjs happens to set -b ast=true, there's no harm done right?

$ echo 'console.log("PASS")' | node bin/uglifyjs -cmb ast
console.log("PASS");
$ echo 'console.log("PASS")' | node bin/uglifyjs -cmb ast=0
console.log("PASS");
$ echo 'console.log("PASS")' | node bin/uglifyjs -cmb code
console.log("PASS");

However,

$ echo 'console.log("PASS")' | node bin/uglifyjs -cmb code=0
undefined

$ echo 'console.log("PASS")' | node bin/uglifyjs -cmb code=0
undefined

That makes sense - it was requested. As long the bash program return value is still 0 indicating success.

Eventually I'd like to see the dump uglify AST patch integrated into the CLI with the -b ast option. The only thing the prevented its inclusion was the inefficiency introduced by the overhead of adding _class to every AST node instance which we do not want in default use.

The only thing the prevented its inclusion was the inefficiency introduced by the overhead of adding _class to every AST node instance which we do not want in default use.

I just had a brief visit at #769 - what does ._class do on top of .prototype.TYPE?

what does ._class do on top of .prototype.TYPE?

They're essentially the same except that ._class has an AST_ prefix. But the point was to output the class name as JSON for every AST node, which is not done for TYPE.

Edit: and to make _class the first entry for each object.

@alexlamsl If you do merge some variant of the dump AST patch into the CLI, please apply these changes: #769 (comment)

Okay, in that case we can safely eliminate that since we have exports["AST_" + type] = ctor; so we know that prefix is constant.

I read the PR and my first reaction was "why isn't this done as JSON.stringify(ast, replacer)" - I agree with your suggestion of preserving order of properties, as they do convey semantics somewhat.

I read the PR and my first reaction was "why isn't this done as JSON.stringify(ast, replacer)"

By all means - whatever works to not introduce more AST object overhead.