Warning "used but not goog.require'd" doesn't make sense here

[supersteves]

I'm loading a CommonJS module in the browser. Externs:

/**
 * @param  {string} module
 * @return {*}
 */
var require = function(module) { return {}; };

Code:

goog.module("test");
let SomeCommonJsMod = require("SomeCommonJsMod");
exports = new SomeCommonJsMod();

Warning:

input0:5: WARNING - 'SomeCommonJsMod' used but not goog.require'd
exports = new SomeCommonJsMod();
          ^

Test case:
https://closure-compiler-debugger.appspot.com/#input0%3Dgoog.module(%2522test%2522)%253B%250Alet%2520SomeCommonJsMod%2520%253D%2520require(%2522SomeCommonJsMod%2522)%253B%250Aexports%2520%253D%2520new%2520SomeCommonJsMod()%253B%250A%250A%26input1%26conformanceConfig%26externs%3D%252F**%250A%2520*%2520%2540param%2520%2520%257Bstring%257D%2520module%250A%2520*%2520%2540return%2520%257B*%257D%250A%2520*%252F%250Avar%2520require%2520%253D%2520function(module)%2520%257B%2520return%2520%257B%257D%253B%2520%257D%253B%250A%26refasterjs-template%26CHECK_REQUIRES%3D1%26LANG_IN_IS_ES6%3D1

I'm not sure what case this warning is trying to assist with, but this isn't it. Something to do with it being a constructor call (see https://github.com/google/closure-compiler/blob/b7abddebe2d9da6cac9827f452ebba2c0e980f93/src/com/google/javascript/jscomp/CheckRequiresForConstructors.java).

It doesn't happen if I write:

goog.module("test");
let SomeCommonJsMod = require("SomeCommonJsMod");
exports = SomeCommonJsMod;

Also If I do this, it doesn't happen:

goog.module("test");
{
  let SomeCommonJsMod = require("SomeCommonJsMod");
  exports = new SomeCommonJsMod();
}

(and is my block-scoped exports line here a valid way of using goog.module? Is it OK to work around this warning - which my project classes as a fatal error - by block-scoping the exports and also the ES6 class definition if any?)

[MatrixFrog]

If you're using a combination of goog.{module,provide,require} and commonJS then it probably makes sense to turn that off: --jscomp_off=missingRequire

[supersteves]

Probably. I also found that adding a constructor annotation suppresses the warning:

goog.module("test");

/** @constructor */
let SomeCommonJsMod = require("SomeCommonJsMod");

exports = new SomeCommonJsMod();

https://closure-compiler-debugger.appspot.com/#input0%3Dgoog.module(%2522test%2522)%253B%250A%250A%252F**%2520%2540constructor%2520*%252F%250Alet%2520SomeCommonJsMod%2520%253D%2520require(%2522SomeCommonJsMod%2522)%253B%250A%250Aexports%2520%253D%2520new%2520SomeCommonJsMod()%253B%250A%250A%26input1%26conformanceConfig%26externs%26refasterjs-template%26CHECK_REQUIRES%3D1%26LANG_IN_IS_ES6%3D1

[blickly]

An alternate way to fix but still get the check would be to define a CommonJS coding convention with the correct implementation of

public String extractClassNameIfRequire(Node node, Node parent);

that understands the CommonJS-style require().

[supersteves]

Another workaround I found was to create a goog.module which aliases the dynamically loaded CommonJS modules:

goog.module("myAlias");
exports = {
    SomeCommonJsMod: require("SomeCommonJsMod")
};

Then to use it:

const SomeCommonJsMod = goog.require("myAlias").SomeCommonJsMod;

Currently I'm using this approach to load React while avoiding polluting window.React.

I don't want to turn off warnings about missing requires, because the codebase is extensive and primarily goog.module/provide based.

[supersteves]

I'm now trying React as a global (window.React), instead of dynamic runtime loading.

See PR #1147

Here I'm using externs traditionally. I still get this "used but not goog.require'd" warning. Should I? If my little PR is accepted, this can be easily worked around using "@Suppress".

[blickly]

If you aren't using goog.provide/goog.require or some other mechanism that
has an associated CodingConvention, then there's no reason to turn on the
missingRequire check at all, and you're better just to turn it off with
--jscomp_off=missingRequire

On Wed, Sep 16, 2015 at 3:24 AM, supersteves [email protected]
wrote:

I'm now trying React as a global (window.React), instead of dynamic
runtime loading.

See PR #1147 #1147

Here I'm using externs traditionally. I still get this "used but not
goog.require'd" warning. Should I? If my little PR is accepted, this can be
easily worked around using "@Suppress https://github.com/suppress".

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

[supersteves]

Understood, however I am using goog.* extensively, while starting to use React (now using window.React, having given up dynamically loading via CommonJS require):

class A extends React.Component { // window.React.Component is defined in externs
}

If it weren't for #1145 and #1125, I'd post a test case.

So I don't want to turn off a warning which is useful for 99% of my codebase.

[blickly]

Ah, OK, got it. I do think it makes sense to have class-level suppressions.

On Wed, Sep 16, 2015 at 9:36 AM, supersteves [email protected]
wrote:

Understood, however I am using goog.* extensively, while starting to use
React (now using window.React, having given up dynamically loading via
CommonJS require):

class A extends React.Component { // window.React.Component is defined in externs
}

If it weren't for #1145
#1145 and #1125
#1125, I'd post a test
case.

So I don't want to turn off a warning which is useful for 99% of my
codebase.

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

[supersteves]

Update: #1366 resolves the last issue discussed. The original issue still stands (confusing / wrong "used but not goog.require'd" warning):

https://closure-compiler-debugger.appspot.com/#input0%3Dgoog.module(%2522test%2522)%253B%250Alet%2520SomeCommonJsMod%2520%253D%2520require(%2522SomeCommonJsMod%2522)%253B%250Aexports%2520%253D%2520new%2520SomeCommonJsMod()%253B%250A%250A%26input1%26conformanceConfig%26externs%3D%252F**%250A%2520*%2520%2540param%2520%2520%257Bstring%257D%2520module%250A%2520*%2520%2540return%2520%257B*%257D%250A%2520*%252F%250Avar%2520require%2520%253D%2520function(module)%2520%257B%2520return%2520%257B%257D%253B%2520%257D%253B%250A%26refasterjs-template%26CHECK_REQUIRES%3D1%26LANG_IN_IS_ES6%3D1%26TRANSPILE%3D1

[Dominator008]

Looks like this is fixed by the recent changes. @MatrixFrog