[CLA][patch] Builds targeting Node.js call wrong version of require() from the bootstrap sequence

[MaxMotovilov]

Bootstrap calls require() from module scope -- in Node.js it resolves not to the global require() function set up by Dojo but to Node.js original version injected into the closure by module loader.

Proposed fix explicitly calls global.require() thus removing the ambiguity.

[MaxMotovilov]

Patch against trunk

[Eugene Lazutkin]

It looks like we assign to global.require using a qualifier, but all calls are unqualified. Most probably it was an omission, when global was added to the assignment, but not to calls.

[ben hockey]

i don't understand - your patch is suggesting changes to the output generated by the build. there is no global.require in the browser so this patch would break all built code in the browser.

what is the problem you are trying to solve?

perhaps what you mean to do is to change dojo/_base/configNode.js so that when injectUrl calls vm.runInThisContext, the require that is in scope will be global.require rather than the require for the dojo/_base/configNode module. something like this:

_base/configNode.js

@@ -66 +66 @@
                         },
 
                         injectUrl: function(url, callback){
+                                var require = global.require;
                                 try{
                                         vm.runInThisContext(fs.readFileSync(url, "utf8"), url);
                                         callback();

[MaxMotovilov]

Replying to neonstalwart:

i don't understand - your patch is suggesting changes to the output generated by the build. there is no global.require in the browser so this patch would break all built code in the browser.

Maybe I'm missing something, but it looks like global is injected in the same way in all environments... For example: ​http://ajax.googleapis.com/ajax/libs/dojo/1.7.2/dojo/dojo.js.uncompressed.js, line 141

what is the problem you are trying to solve?

Get the builds to work in node.js. Currently they don't. Try it.

perhaps what you mean to do is to change dojo/_base/configNode.js so that when injectUrl calls vm.runInThisContext, the require that is in scope will be global.require rather than the require for the dojo/_base/configNode module.

I am not sure whether this will resolve the issue, but it will definitely overwrite Node's own require which my proposed solution doesn't.

Again, the underlying problem that I was trying to solve is that module pre-loading request in the generated bootstrap code calls the wrong require (e.g. see line 1826 in the version of the bootstrap referenced above). Since Dojo's require is explicitly injected into global as part of the same file, I don't see why it should be referenced via an unqualified name (thus conflicting with the name injected into module scope by the Node) just a few lines below.

[ben hockey]

global is a local var it is not a global. see ​http://bugs.dojotoolkit.org/browser/dojo/dojo/trunk/dojo.js#L66 which is one very long var statement that includes global.

my solution does not overwrite node's require - it shadows node's require (which is shadowing the global require) so that built code will get the appropriate require it expects.

the call to require in the built code needs to be unqualified (ie NOT global.require) because global does not exist in browsers. my solution simply puts the appropriate require in scope for code that is executed via vm.runInThisContext and hence fixes the cause of this problem - the require that is in scope for the call to vm.runInThisContext should be global.require rather than the require provided by node to the configNode module.

[MaxMotovilov]

Replying to neonstalwart:

global is a local var it is not a global. see ​http://bugs.dojotoolkit.org/browser/dojo/dojo/trunk/dojo.js#L66 which is one very long var statement that includes global.

Right :( That's what I was missing -- that IS a long var statement.

my solution does not overwrite node's require - it shadows node's require (which is shadowing the global require) so that built code will get the appropriate require it expects.

Unfortunately it doesn't seem to work. I tried it in my test code and, even though configNode.config does get called, at the key point in the loader require is still that of the Node, not the one that should have been injected. I don't see how injectUrl() could possibly affect generated loader code anyway as this code is in the file passed to Node on command line rather than in one of the modules loaded dynamically.

the call to require in the built code needs to be unqualified (ie NOT global.require) because global does not exist in browsers. my solution simply puts the appropriate require in scope for code that is executed via vm.runInThisContext and hence fixes the cause of this problem - the require that is in scope for the call to vm.runInThisContext should be global.require rather than the require provided by node to the configNode module.

Well it doesn't -- for the reasons outlined above :( Ideas?

[Rawld Gill]

In [28431]:

hijack require in loader when has host-node so that built layers in bootstrap load; added basic node build profile; fixes #15205; !strict

[Rawld Gill]

demo of concepts discussed on this ticket

[Rawld Gill]

In [28432]:

improved basic node profile by removing unneeded special boot sequence; refs #15205

[Rawld Gill]

The core issue is require must reference AMD require, but this is only a problem

• after the loader is defined
• in the loader resource...which may have a bootstrap and/or cached modules in built versions

Notice that vm.runInThisContext does not pollute the environment with node's require function. Therefore, when a resource is injected by the loader, everything works fine. It is only the loader resource itself that is the problem in the node environment because this resource is loaded by node's loader which includes the lexical variable require defined by node's loader.

There was a tiny changed to account for this; see [28431].

I also added a basic node profile as part of this ticket.

I provided a sample to prove/demo all concepts. See the readme in sample.zip, attached.

Lastly, I'm unclear as to the value of "building" for node. Latency and bandwidth isn't a problem. I expect no significant performance gain by building for node. I'd be interested to hear alternative views on this.

Thanks for all the help on this ticket; it allowed me to quickly zero in on the core issue.

[MaxMotovilov]

Lastly, I'm unclear as to the value of "building" for node. Latency and bandwidth isn't a problem. I expect no significant performance gain by building for node. I'd be interested to hear alternative views on this.

The value is in easy distribution of a self-contained utility script. In my specific case, I have a template engine that sees primary use on the client side (i.e. in the browser); it does however have a capability to automatically extract a dictionary of localizable string fragments from a specified set of templates. This extraction is really a build-time function that is best performed by a self contained script that can be easily run as part of an automated build system -- hence the need for a Dojo build (the resulting single layer is simply run as node collection-script.js args...)

[MaxMotovilov]

Oh, and thanks a lot for resolving this nagging problem! I will try your changes shortly.