Cannot use -D for @define constant within a module

[jleyba]

Repro case:

// a.js
/** @define {string} */ var FOO = 'default foo';
export function test() { console.log(FOO); }

// b.js
import * as a from './a';
a.test();

java -jar compiler.jar --js *.js \
    --language_in=ECMASCRIPT6_STRICT \
    --language_out=ECMASCRIPT5_STRICT \
    -D "FOO='hi'";

WARNING - unknown @define variable FOO

0 error(s), 1 warning(s)
'use strict';var module$a={},FOO$$module$a="default foo";function test$$module$a(){console.log(FOO$$module$a)}module$a.test=test$$module$a;module$a.test();

Things work if you use the mangled name:

java -jar compiler.jar --js *.js \
    --language_in=ECMASCRIPT6_STRICT \
    --language_out=ECMASCRIPT5_STRICT \
    -D "FOO\$\$module\$a='hi'";

'use strict';var module$a={},FOO$$module$a="hi";function test$$module$a(){console.log(FOO$$module$a)}module$a.test=test$$module$a;module$a.test();

[Dominator008]

Technically the compiler is doing the right thing. According to https://developers.google.com/closure/compiler/docs/js-for-compiler, @defines are only allowed in the global scope. The meaning of @define in a "module scope" is a bit unclear, but maybe we should consider supporting this use case by either:

1. Treating all @defines as truly global and not mangling the names.
2. Treating @defines as module-local, which requires updating the arguments passed in with -D.

I'd prefer going with option 1. We should also update the documentation as ES6 modules are becoming popular.

[ChadKillingsworth]

As I mentioned on the PR, I'm opposed to option 1 as it will block proper scoping of modules and prevent two different versions of a module from being used in the same compile.

There's also a couple of other options:

1. Mangle the names, but when matching to replace the value use the original name.
2. Process defines before rewriting modules.

[jleyba]

Or 3) make it an error to have a define in a module
On Tue, Mar 1, 2016 at 4:38 AM Chad Killingsworth [email protected]
wrote:

As I mentioned on the PR, I'm opposed to option 2 as it will block proper
scoping of modules and prevent two different versions of a module from
being used in the same compile.

There's also a couple of other options:

1. Mangle the names, but when matching to replace the value use the
   original name.
2. Process defines before rewriting modules.

—
Reply to this email directly or view it on GitHub
#1601 (comment)
.

[blickly]

I think that goog.define() will work inside ES6 modules, which is generally preferred over naked @defines anyway since it can also work in uncompiled code.

[jleyba]

goog.define() only works in uncompiled code if you include closure's base.js

[ChadKillingsworth]

I thought of a better way to handle this: during module rewriting, rewrite the define flags in a similar way that rewrite type nodes.

[concavelenz]

Here is what I had in mind:
We need to add support for modules, a define for a module would look something like this:

-D "path/to/module.js:NAME=1"

for the define to be visible outside the module, it would need to be exported separately. Defines in modules should only be allowed on const declarations:

/** @define {boolean} */
const FOO = false;

goog.define also needs to be modified for ES6 modules as they don't know their own name so it is difficult to look them up at runtime.

Alternately, we require all @define values to be globally unique but that would eventually going to cause problems.

[jplaisted]

Probably worth revisiting this as modules (including ES6 modules) gain popularity. fwiw goog.define does not work in a Closure module either.

goog.module('my.module.namespace');

/** @define {number} */
goog.define('my.module.namespace.DEFINE', 0);

There's two issues with the above code:

1. goog.module does not create the namespace, nor does goog.define. Thus the compiler will emit my.module.namespace.DEFINE = 0;, which is an error since my.module.namespace is not defined.
2. If it did make the namespace (either intentionally or you called goog.module.declareLegacyNamespace) then it is creating a global, which does against the idea of modules.

Ideally we'd have some define mechanism that truly fits with modules, i.e. creates an expression result for the value only. For Closure Library I'd like to add a goog.defineLocal method (too late to rename goog.define) that returns the value rather than defining a global symbol. But the issue is non-Closure defines need new syntax. imo it'd be nice if goog.defineLocal took any string, not just valid JS identifiers. That way you could easily incorporate the file name like project/foo/bar/baz.js:DEFINE. Question is, how do you make this work for the non-Closure define? Seems like people don't want new syntax there. But we can't change the behavior of the existing @define. Do we have some new @define(NAME) {boolean}? e.g.

/** @define(path/to/file:MY_DEFINE) {number} */
export const MY_DEFINE = 0;

imo these module define names still need to be unique. You shouldn't be allowed to reuse them. Keeps the --define flag clear, just a string match. i.e. rather than have module defines have scoped names and change the syntax of --define to select which module you mean, instead have module defines need globally unique names and then --define need not change. While this particular aspect doesn't fit with the module idiom, I think the clarity is provides is worth it.

tl;dr need help deciding on what the expected behavior and syntax is. Then we can move to implement. But there's been disagreement between myself, @shicks, and @concavelenz as to what the @define needs to change to in a module.

[shicks]

Since we don't have a clear decision here, I'll offer some more bikeshedding.

For @define in a module, go with what @concavelenz suggested - /** @define {boolean} */ const FOO = false; specifies a define called path/to/file.js:FOO. Obviously we'd need to shore up how the relative path is constructed - it suddenly matters what directory you build from, which is possibly problematic. For goog.modules we could use the module identifier with a colon, but that doesn't seem very forward-looking at this point (though goog.module.declareNamespace could extend its life a bit). For Closure defines, rather than a new function, we could just look at the name and see if it has a non-identifier character in it. If so, don't define it globally and instead return it:

const FOO = goog.define('path/to/my:FOO', false);

The name needn't necessarily be the same as the actual path - it just needs to have a non-identifier character in it, which by convention could be slashes and colons. Otherwise it's an arbitrary globally-unique string.

[ChadKillingsworth]

@shicks I'd recommend relying on module resolution to locate the file. That handles all the relative/absolute/goog.module issues.

[jplaisted]

@shicks not sure I like having ES6 modules have their path as the define and goog.modules having a string constant representing the path as a define (edit: to be clear, I'm not okay with this because that string constant can really be anything). I'd like for these to be consistent. Which is why I'd rather have a new method like goog.defineLocal or goog.moduleDefine or something that takes a string and a value like goog.defineLocal('FOO', false). Then you can use the path style that ES6 modules are proposed to use.

@ChadKillingsworth I think his point is if you take your compiler defs and then build a directory up from where you normally do your defs are broken. But hey, welcome to paths. I think is reasonable. However it might be unreasonable how it works with other flags. I think if you pass --js=relative/my/file.js and define -d /absolute/relative/my/file.js:DEF=false while building from /absolute/ the compiler won't handle these with the way module resolution works today. But that's speculation.

In any case its the best we have. I guess if you make your POV that the flag file is another ES6 module in your build directory that can reference others it makes more sense.

[jplaisted]

Let's maybe ignore the Closure Library method for now. Thinking further a string constant might be necessary, otherwise how do you resolve the path in the browser? Unless we do namespace:value, which @shicks seemed to reject.

[l1bbcsg]

Am I correct in understanding @defines are currently unusable with modules?

Since defines are expected to be in global scope, a file containing defines can only be a script, but at the same time it must be included in the compilation which in case of module-managed projects means something has to depend on it which requires it being a module.

The two workaround I found so far are --dependency_mode LOOSE to pick up an unreferenced file and mixing in a goog.provide, both of which are really unwanted.

Is there a better way currently?

Any progress on resolving the problem itself?

[jplaisted]

You're correct in your assumptions, and I haven't had time to prioritize this.

[ray007]

My vote would go to "treat all defines as truly global and do not mangle their names".
If I supply a -D argument to a build process, I do not care in which file the define is located.

[arv]

Still want this to be resolved...

One more idea how to deal with this; Let -Dxyz=123 define a mapping from xyz to 123. When we see a @define named xyz in any module we replace the value. That is it. That means that if there are multiple defines named the same thing they will share the value. In practice I don't think this is an issue. This also means that files need to import and export the @defined bindings but it leads to less confusion and works better with tools such as eslint and vscode anyway.

[lauraharker]

This maps to internal issue http://b/117789111, which Steve has fixed.

@shicks can we close this issue and update the external documentation?

You can write:

goog.module('my.module');
/** @define {number} */
const MAX_SIZE = goog.define('some.arbitrary.string', 42);

with the corresponding flag: ---define some.arbitrary.string=43. I believe this still declares some.arbitrary.string globally (is that right @shicks?)

[arv]

@lauraharker How does this work with ES modules? With CJS?

[shicks]

We've made some improvements, but the story is pretty different depending on whether or not you're using Closure Library. TL;DR: goog.define works, but raw @define doesn't really.

With Closure Library

goog.define now works correctly in modules by returning its value to be stored in a module-local variable, with the caveat that it does still export global values for the time being. In short, you can write in a module

/** @define {number} */
const FOO = goog.define('some.ns.FOO', 1);

and then set it from the command line with --define some.ns.FOO=42. The string identifier must be a valid JS qualified name, but aside from linking the command-line flag to the specific definition, it has no other use (once we stop exporting it as global—which will happen in the next few months, once we've cleaned up everything that would break).

This has sort of worked in the past, but in an unsupported way: in compiled code it defines a module-local with no global, while in uncompiled code it defined a global. The main breakages that need to be fixed before we can stop exporting the global are based on folks exploiting this inconsistency to change @defined values in uncompiled tests.

Without Closure Library

Without goog.define, the story is unfortunately a bit more complicated. You can't just write /** @define {number} */ const FOO = 1; and -DFOO because module rewriting happens before defines are processed, so you'd need to write a munged name on the command line. This munging of course is an implementation detail and depends on the kind of module (goog, ES, CJS) as well as whether it's exported or not. I don't know of a good way to determine the munged name (short of changing the compiler to print allDefines.keySet() at the end of ProcessDefines#overrideDefines), nor do I think it's a particularly good idea. It would be possible to rearrange it so that it's keyed on the unmunged name, but I worry that that would break folks that already are (unwisely or not) depending on this munging.

Independent of this, it's still an error to define the same symbol twice. We could revisit that, but I'm not convinced that would be a good idea.

[shicks]

Whether or not this satisfies the requirements of this issue is unclear to me.

[arv]

Whether or not this satisfies the requirements of this issue is unclear to me.

Doesn't sound like it is working to me 😉 .

[ray007]

How about something like -Dname@module=value?

[ctjlewis]

Alternately, we require all @define values to be globally unique but that would eventually going to cause problems.

That should be fine - they're all defined in a globally unique way (by passing to the compiler at compile-time). The @define variables are always global by definition, I fail to see the conflict in adding this.

[ctjlewis]

I've finally managed to find a solution to this that will not throw a goog is not defined VM error at runtime, will not throw a compiler error for overriding goog, and does not require actually depending on the Closure Library source (which will not work with ES6 import while also successfully compiling).

/** global.js */

// overrides `goog.define` for NodeJS runtime, where it does not exist
if (typeof goog === 'undefined' && typeof globalThis !== 'undefined') {
  globalThis.goog = {
    define: (n, v) => v,
  };
}

export var PRODUCTION = goog.define('compiler.globals.PRODUCTION', false);

Where the compiler is called with google-closure-compiler -D compiler.globals.PRODUCTION=true for release output. Use import { PRODUCTION } from '../globals.js' as needed.

[adrianholovaty]

The super-hacky solution I've come up with for this:

• Before compilation starts, our build tool does a search-and-replace for DEBUG=true in the codebase and changes it to DEBUG=false.
• Closure Compiler runs, compiling our code for production.
• After compilation, the build tool reverts the search-and-replace (converting DEBUG=false back to DEBUG=true).

This avoids the use of @define entirely. Sorry to suggest something so hacky, but it gets the job done.

[ctjlewis]

I left a hacky, but working solution above if you want to actually use @define - you just need to use goog.define('fully.qualified.name', ...), where you have to use a fully qualified module identifier for the goog.define name.

Hacky but it works while still relying on the compiler, and I also posted a polyfill for goog.define so you can actually execute the script without needing the Closure Library in-scope.

[ray007]

* Before compilation starts, our build tool does a search-and-replace for `DEBUG=true` in the codebase and changes it to `DEBUG=false`.

For the debug build I have special comments like //@DBG@ that get replaced only during debug build.

Pro: quicker/easier to write (imho), and needs no if(DEBUG)...
Con: debug-code does not get highlighted in the editor