Differentiate Internal/External Require Calls for Common JS Modules

[kegsay]

I want to compile this very simple piece of code:

// test.js
var crypto = require("crypto");
console.log(crypto.createHash("md5").update("foo").digest("hex"));

I know the compiler needs to know where the crypto library is, so I'm passing it in via --externs (I do not want it to be compiled along with my code, I just want it bluntly passed through).

However, I cannot get this to compile at all. I'm compiling it with:

$ java -jar compiler.jar --js="test.js" --process_common_js_modules --common_js_entry_module "test.js" --externs closure-compiler/contrib/nodejs/crypto.js

ERROR - required entry point "module$crypto" never provided

1 error(s), 0 warning(s)

where closure-compiler/contrib/nodejs/crypto.js is the file in this git repo. I've also tried using globals.js to no avail. What am I doing wrong? Looking at the source, it's producing this error because module$crypto is not contained in modulesByProvide in src/com/google/javascript/jscomp/Compiler.java:1534, which seems to just be looping over the files I give it in --js and not the externs files.

Version:

Closure Compiler (http://github.com/google/closure-compiler)
Version: v20150505
Built on: 2015/05/06 13:09

[dimvar]

Try omitting this line:

var crypto = require("crypto");

The externs should be available to all modules without explicitly requiring them. You will have to use the full name of the module instead of just crypto. If you want to use a short name, use goog.scope in test.js.

[kegsay]

So you're telling me there is no way to get closure compiler to do this without altering my code to make it closure-specific?

[nicks]

Right. closure-compiler will not work with native node modules.

[ChadKillingsworth]

The problem is the require statement gets expanded by the compiler and there is no way to denote that certain require statements are external.

What happens if you alias require?

var externalRequire = require;
/** @suppress {duplicate} */
var crypto = externalRequire('crypto')

[kegsay]

Aliasing require does indeed work, which is a great start: thank you!

So overall, there isn't any way to specify where closure-compiler should look to resolve references like this? E.g. A compiler option to say "If you see a require call, look in some/dir/here"? Clearly the require call is already doing something like this since it can resolve paths correctly. It seems surprising that closure-compiler has this half-support for require calls (in that it resolves files/paths correctly, just not anything native like crypto or anything in node_modules).

The benefit to doing this would be that it would allow type checking on Node.js projects without any code alteration. If you're open to the idea of supplying a compiler option, I would be interested in making a PR for this.

[ChadKillingsworth]

We're open to improvements like this, but it may be trickier than you think.

During a very early pass in the compiler, require statements are expanded like a macro to simply be goog.require('module$crypto'). The common js and AMD exports are also expanded to be goog.provide('module$crypto'). The compiler currently has no way to recognize at this point that there is a require statement without a corresponding provide.

The difficult part would be when should a require without a provide be considered an external call and when should it be considered an error?

[kegsay]

Am I right in suggesting that the macro-like expansion of require statements was done under the assumption that you would be requireing your own source code which you wanted to compile? That seems to be how the compiler is treating them currently. Given that aliasing require works exactly as I'd expect without any surprising behaviour, I think it would make sense to simply not expand require statements when the location is unknown (e.g. because it is native / in node_modules / just not supplied to the compiler to compile).

Yes, I agree that it is difficult to know if it supposed to be an external call or if it is just a typo and should be considered an error. The compiler does not have enough information to make a call on that. Would it be possible to add an annotation to explicitly declare the require as an external call?

This would mean that the proposed additional functionality would be to not expand out require statements if and only if there is a new JSDoc-style annotation (e.g. @externalRequire) marked on the require statement. This would make this change low-risk given it would only take effect when the new annotation was applied.

[ChadKillingsworth]

In general, we don't like to add annotations. Also, how is adding an annotation any different than aliasing the require statement? Both require modifications to code.

Also, there are cases where a node_modules required library should be included as source. Namely any npm installed library that is compatible with Closure-compiler could fall into this category. This can include libraries from private registries.

[kegsay]

Aliasing the require statement is more intrusive than comments as it makes the code more obfuscated (people naturally see require("whatever") and immediately get it if they're a Node dev, but it throws you off if you see externalRequire("whatever") even if it is aliased a few lines up. Comments remove this obfuscation and makes the intent clearer that it's just a flag for some other tool:

/** @externalRequire */ 
var crypto = require("crypto");

For cases where a node_modules library should be included as source, I don't believe the compiler has any way of managing that currently without referencing the node_modules folder and including the relevant module with --js, so my suggestion wouldn't change that in any way (and is also why I quite like mentioning the keyword external in the annotation name given there is --externs which functions in a very similar way).

Is there any particular reason for disliking new annotations? On the contrary, annotations are the only way anything non-closure specific can interact sensibly with the compiler. I can understand that they should be used carefully where the compiler doesn't have enough information to make a decision, but I feel that they can be extremely valuable for cases such as this.

I can always write a script to alias all of the require statements I wish to exclude, but I was hoping that it would be a generic enough problem to want to solve in the compiler itself, given Node.js projects are just so close to being able to play nicely with the compiler.

[ChadKillingsworth]

How is this case handled by the Require JS optimizer? It seems like the compiler should aim for parity with already understood methodologies.

Also, I would suggest writing up a design document and posting for comments in the Closure-Compiler-Discuss group. Most of the compiler maintainers do not use common js modules, so you'll get more feedback there. Here's an example design document: https://gist.github.com/ChadKillingsworth/b86a4cffaa71571b5d01

[kegsay]

Node.js preferentially loads up core modules, then knows where to look based on the first few characters of the provided string (lack of ./ or / indicates it's a module and should look in node_modules). The documentation is actually really helpful for outlining the algorithm used here: https://nodejs.org/api/modules.html#modules_core_modules

As a result, it can tell the difference between native libs like crypto, npm packages like q and random source files like ../data/something.js. If you wanted to get closure compiler working with some npm packages (for example q), then you'd drop the annotation on the require statement but would then run into problems knowing where to look (the compiler would have to implement the lookup algorithm for node_modules at https://nodejs.org/api/modules.html#modules_loading_from_node_modules_folders )

I'll write up a design document this weekend.

[kegsay]

Posted to https://groups.google.com/forum/#!forum/closure-compiler-discuss

Design doc draft for reference:
https://gist.github.com/Kegsay/2cb999a78dbd2b8bda95

[kegsay]

I'm still keen to get this feature implemented.

I originally fixed my problem by using Flowtype instead for type checking (which does resolve require() calls correctly), but the kicker is that in order to add type annotations you need to transpile since the annotations are not valid Javascript. It does support an ugly commented form, but what I really want is for it to use JSDoc so I can use 1 true type annotation for both type checking and documentation. Closure compiler does this really well. I ended up writing a small tool to allow me to write JSDoc for type annotations in Flowtype and that works reasonably well, but has fundamental limitations to how aggressively I can annotate.

It looks like @ChadKillingsworth has been the most enthusiastic one to also get this implemented, and may have started working on this already? Assuming this is the case, is there anything I can do to push the process along?

[atroche]

@ChadKillingsworth what if the compiler kept a hard-coded list of node libraries, and left the calls to require alone when --module_resolution NODE is set? Or I suppose it could automatically alias the require calls.

[atroche]

Oh, excuse me, I only just found this issue: #1382

And I agree that having a flag is a better system 👍

Mainly because it makes it easier to use popular npm modules that use core libraries like http without having to modify them (to alias require).