#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)
Change History (14)
Changed 9 years ago by
Attachment: | node-require-fix.patch added |
---|
comment:1 Changed 9 years ago by
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 9 years ago by
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.
comment:3 follow-up: 4 Changed 9 years ago by
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 }, 67 67 68 68 injectUrl: function(url, callback){ 69 var require = global.require; 69 70 try{ 70 71 vm.runInThisContext(fs.readFileSync(url, "utf8"), url); 71 72 callback();
comment:4 Changed 9 years ago by
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
callsvm.runInThisContext
, therequire
that is in scope will beglobal.require
rather than therequire
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 follow-up: 6 Changed 9 years ago by
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 Changed 9 years ago by
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 includesglobal
.
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 appropriaterequire
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
) becauseglobal
does not exist in browsers. my solution simply puts the appropriaterequire
in scope for code that is executed viavm.runInThisContext
and hence fixes the cause of this problem - therequire
that is in scope for the call tovm.runInThisContext
should beglobal.require
rather than therequire
provided by node to the configNode module.
Well it doesn't -- for the reasons outlined above :( Ideas?
comment:9 follow-ups: 10 11 Changed 9 years ago by
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 Changed 9 years ago by
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 Changed 9 years ago by
Oh, and thanks a lot for resolving this nagging problem! I will try your changes shortly.
comment:12 Changed 9 years ago by
Milestone: | tbd → 1.8 |
---|
Patch against trunk