Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#15205 closed defect (fixed)

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

Reported by: MaxMotovilov Owned by: Rawld Gill
Priority: undecided Milestone: 1.8
Component: BuildSystem Version: 1.7.2
Keywords: Cc:
Blocked By: Blocking:

Description

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.

Attachments (2)

node-require-fix.patch (3.2 KB) - added by MaxMotovilov 8 years ago.
Patch against trunk
simple.zip (3.5 KB) - added by Rawld Gill 8 years ago.
demo of concepts discussed on this ticket

Download all attachments as: .zip

Change History (14)

Changed 8 years ago by MaxMotovilov

Attachment: node-require-fix.patch added

Patch against trunk

comment:1 Changed 8 years ago by Eugene Lazutkin

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

comment:2 Changed 8 years ago by 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.

Last edited 8 years ago by Eugene Lazutkin (previous) (diff)

comment:3 Changed 8 years ago by 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

     
    6666                        },
    6767
    6868                        injectUrl: function(url, callback){
     69                                var require = global.require;
    6970                                try{
    7071                                        vm.runInThisContext(fs.readFileSync(url, "utf8"), url);
    7172                                        callback();

comment:4 in reply to:  3 Changed 8 years ago by 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.

comment:5 Changed 8 years ago by 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.

comment:6 in reply to:  5 Changed 8 years ago by 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?

Last edited 8 years ago by MaxMotovilov (previous) (diff)

comment:7 Changed 8 years ago by Rawld Gill

Resolution: fixed
Status: newclosed

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

Changed 8 years ago by Rawld Gill

Attachment: simple.zip added

demo of concepts discussed on this ticket

comment:8 Changed 8 years ago by Rawld Gill

In [28432]:

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

comment:9 Changed 8 years ago by 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.

comment:10 in reply to:  9 Changed 8 years ago by 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...)

comment:11 in reply to:  9 Changed 8 years ago by MaxMotovilov

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

comment:12 Changed 8 years ago by bill

Milestone: tbd1.8
Note: See TracTickets for help on using tickets.