Allow comments in flagfile

[alexeagle]

The --flagfile argument to the compiler is a convenient way to package up the many options needed.
However, the options can be surprising, so the flag file would strongly benefit from being able to add comments:

• what this option does
• explanation for why it's used in the project
• a TODO to change the option to another value, link to a bug

[ChadKillingsworth]

@alexeagle Would a JSON config file similar to webpack and typescript be better?

[FrozenPandaz]

I just started playing with the closure compiler (with @alexeagle's help).

But my 2 cents would be that being a Javascript tool, using a Javascript notation makes sense to me.

Note: tsconfig.json's let you use comments but package.json's do not

[kylecordes]

Typically a JSON configuration format would not accommodate the thing @alexeagle was hoping for, comments intermixed with configuration. As I understand some systems work around this by accepting yet another format which is not JSON per se, but a kind of hybrid JSON it also allows comments.

https://www.npmjs.com/package/comment-json

However, for the sake of moving things along, swapping the whole thing over to a completely different way of specifying configuration sound like a lot more work than just tolerating comment lines in the existing format. I don't have a "vote" but if I did I would encourage the effort be spent on numerous essential features instead of migrating to a different configuration format.

[FrozenPandaz]

Yeah, not an essential feature but I think it can easily be converted...

By the way, this is what i came up with for how it might look..

{
	"compilation_level": "ADVANCED_OPTIMIZATIONS",
	"language_out": "ES5",
	"js_output_file": "dist/bundle.js",
	"create_source_map": "%outnamee%.map",
	"variable_renaming_report": "dist/variable_renaming_report",
	"property_renaming_report": "dist/property_renaming_report",
	"warning_level": "QUIET",
	"rewrite_polyfills": false,
	"entry_point": "./aot-built/app/client.aot.js",
	"dependency_mode": "STRICT",
	"output_manifest": "dist/manifest.MF",
	"files": [
		"node_modules/@angular/core/@angular/core.js",
		"node_modules/@angular/common/@angular/common.js",
		"node_modules/@angular/forms/@angular/forms.js",
		"node_modules/@angular/http/@angular/http.js",
		"node_modules/@angular/router/@angular/router.js",
		"node_modules/@angular/platform-browser/@angular/platform-browser.js",
		"externs/zone_externs.js",
		"externs/angular_testability_externs.js"
	],
	"js_module_roots": [
		"node_modules/@angular/core",
		"node_modules/@angular/common",
		"node_modules/@angular/forms",
		"node_modules/@angular/http",
		"node_modules/@angular/router",
		"node_modules/@angular/platform-browser"
	]
}

[alexeagle]

A JSON config file could simply be require()d and passed to the API:
https://github.com/google/closure-compiler-npm#specifying-options
so I think that already works. Allowing comments there might be nice but it's a different issue.

For correct layering, I think the closure-compiler package should not know about the npm CLI? So any changes here should be in terms of the java-based API exposed by compiler.jar.

Indeed this is not that important, so I hope we don't do more than just dropping lines as the file content is read.

[ChadKillingsworth]

Just last week my team members proposed a project config file to help with tooling and IDE plugins - that's why I asked.

The compiler package should NOT know about the npm cli. But the npm cli is where we really care about these issues.

I'm probably not going to be spending time on a flagfile that is hard to maintain and use in other tools - that's why I'm asking. I really only concern myself with the NPM ecosystem.

Or are you wanting to use the flagfile from another tooling system? Like Bazel?

[alexeagle]

The flagfile is just an intermediate state. It doesn't support globs so it isn't comprehensive anyway, at least until the compiler can resolve paths from disk to files that weren't presented on the command line. And eventually from Bazel we will just construct a full command line (like rules_closure) This issue is really low priority, I might contribute it anyway though.

…

On Sat, Apr 8, 2017, 9:15 AM Chad Killingsworth ***@***.***> wrote: Just last week my team members proposed a project config file to help with tooling and IDE plugins - that's why I asked. The compiler package should NOT know about the npm cli. But the npm cli is where we really care about these issues. I'm probably not going to be spending time on a flagfile that is hard to maintain and use in other tools - that's why I'm asking. I really only concern myself with the NPM ecosystem. Or are you wanting to use the flagfile from another tooling system? Like Bazel? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2413 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC5I7UZCxjXiTSarFSHFFjRlu8rL2uwks5rt6R9gaJpZM4M3S-1> .

[ChadKillingsworth]

The command line does support globs. I thought the flagfile did too. Slightly different format from Node though.

[alexeagle]

You're right, I hadn't read the docs on the wiki. My flagfile is very nice now
https://github.com/alexeagle/closure-compiler-angular-bundling/blob/master/closure.conf
(you might be interested to see how Closure is used in that repo, this is the model for how Angular apps will do it)

I'll probably send a PR for comments in the flagfile.

[ChadKillingsworth]

On your config file, I really would expect angular to be using the NODE module resolution algorithm. --module_resolution=NODE. See https://github.com/google/closure-compiler/wiki/JS-Modules#module-resolution-mode

Also, make sure you track the changes in #2343 (which have landed internally). That enables absolute paths to work properly with --js_module_root flags.

[kylecordes]

@ChadKillingsworth Yes, right - but see my comment in the other issue #2423. NODE doesn't know about the fancy new es2015 package.json field. ( @alexeagle - is that feature of the Angular build output, stable enough to ask Closure to know about it yet?)

[alexeagle]

It would be interesting for Closure to understand the alternate main fields such as esnext:main and es2015. But these aren't standardized anywhere, and closure usually avoids anything non-standard.

@ChadKillingsworth we just announced a package format for Angular apps to follow that allows them to work with Closure Compiler, and they also have to do a particular pattern to workaround the fact that ES2015 files are not the "main" main.
(as I'm sure you know, Closure does a much better job optimizing ES2015 code, eg. classes, which is why we don't hand it the primary files which are ES5)

[ChadKillingsworth]

Those alternate field status' are "loosely" defined everywhere. We only look at the main field right now - simply because I haven't gotten around to writing in more support. I know that module and browser need to be honored, but I haven't done that yet.

If a major framework like angular wants to add support for other fields, I'm completely open to do that. We just need to define the order of precedence.

[ChadKillingsworth]

If you can get webpack to follow suite - then that only adds more credence to this.

[alexeagle]

That would be awesome for us. /cc @jasonaden who wrote the packaging spec for Angular.

[jasonaden]

@ChadKillingsworth @alexeagle So Webpack has a configuration option for this called "resolve.mainFields". They default to ["browser", "module", "main"] but this can be easily updated.

I don't think Webpack will directly support the es2015 key as they don't parse es2015 by default. You need to add the babel loader in order to support it, at which point modifying resolve.mainFields would work.

[kylecordes]

Vaguely related: rollup comes with a note module resolver. That resolver does not yet support the es2015 key, for the same reasons as discussed earlier here: That they are aiming to support things that are at least somewhat official standards. I made an alternative resolver that does understand these files, suitable for use with angular:

https://www.npmjs.com/package/rollup-plugin-node-resolve-angular

This is a feasible approach for rollup or webpack (although the letter does not need it, see @jasonaden comment about the configuration option). It's not feasible for Closure because Closure does not have (that I know of) any way to write a plug-in resolver.