#16414 closed defect (fixed)
dojo/node has issues with loading some modules
Reported by: | Kitson Kelly | Owned by: | Colin Snover |
---|---|---|---|
Priority: | undecided | Milestone: | 1.8.5 |
Component: | Core | Version: | 1.8.1 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
Sometimes dojo/node
runs into issues loading modules that detect an "AMD" environment, but are being loaded by the Node module loader (e.g. http://momentjs.com/) because they "see" that define()
is defined in the global scope.
I am attaching a patch that fixes this by creating a separate closure and undefining define for that scope. Modules that I previously had issue with now load fine.
Attachments (3)
Change History (27)
Changed 8 years ago by
Attachment: | dojo_node.patch added |
---|
comment:1 Changed 8 years ago by
Owner: | set to Colin Snover |
---|---|
Status: | new → assigned |
comment:2 Changed 8 years ago by
Owner: | changed from Colin Snover to Kitson Kelly |
---|---|
Status: | assigned → pending |
If the modules support AMD shouldn’t you be loading them directly?
comment:3 Changed 8 years ago by
Status: | pending → new |
---|
I ran into this when using some Node modules that were depending on other node modules that were AMD compatible.
For example, emailjs depends on Moment.js, emailjs isn't AMD aware, but when it goes to require in Moment.js, Moment.js sees define()
and thinks it is AMD, which then causes emailjs to not to load properly.
We could tell everyone to stop using CommonJS modules. I know it would make my life easier.
comment:4 Changed 8 years ago by
Owner: | changed from Kitson Kelly to Colin Snover |
---|---|
Status: | new → assigned |
Didn't realise that this was still assigned to me after I provided more information. Colin, do you have an opinion? I keep having to patch dojo in order to get some of my projects to load properly and would like to get it checked in. I will work on a test case as well that will demonstrate the problem.
comment:5 Changed 8 years ago by
I am not sure how this patch would work correctly since you appear to be changing the global define once and for all. An alternative approach (same theory) exists at https://github.com/csnover/dojo2-core/blob/master/node.js so please feel free to take whatever works and land it, or if you want me to do it I can, it just depends on who gets there first.
comment:6 Changed 8 years ago by
Let me work up a proper test case that exhibits the "broken" behaviour (in that loading a CommonJS module that requires in a module that detects AMD based on the presence of define).
Looking at my patch again, I think I just got lucky somehow. It was working for me but it shouldn't.
I created another "prototype" (http://jsfiddle.net/kitsonk/y5wnF/) and the masking appears to work. Creating it a closure might be unnecessary, but I do like that it reduces potential "interference".
Your dojo2-core works as well...
I have attached another patch based upon my "fiddle" research.
comment:9 Changed 8 years ago by
Milestone: | tbd → 1.8.4 |
---|
comment:13 Changed 8 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
normalize
looks wrong... every relative id is resolved relative to baseUrl
- i don't think that's right. if in the app/foo module i wanted 'dojo/node!./aNodePkg/bar'
it will resolve as 'aNodePkg/bar'
rather than 'app/aNodePkg/bar'
as i intended.
Changed 8 years ago by
Attachment: | 16414.diff added |
---|
comment:15 Changed 8 years ago by
Even though I landed the change, the normalize was from csnover's dojo2 prototype. I guess the question is, do we even need to normalize the MIDs actually? Without any normalization, the MID is simply passed to the node require, which resolves to where the script was invoked from, which has been the expectations in my development to date.
For example, given:
app/index.js app/lib/app/app.js app/lib/dojo/ app/node_modules/express/
Where index.js
loads Dojo and requires in lib/app/app.js
and you execute node .
then dojo/node!./node_modules/express/
would load Express.
comment:16 Changed 8 years ago by
it works because your structure happens to accidentally work - baseUrl + './node_modules/express/'
turns out to be where express lives. move index down a few segments (eg app/foo/bar/index.js
) and then try loading dojo/node!../../node_modules/express/
- baseUrl + '../../node_modules/express/'
is NOT where express lives.
comment:17 Changed 8 years ago by
Milestone: | 1.8.4 → 1.8.5 |
---|
Moving incomplete issues to next minor release.
comment:19 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
comment:22 Changed 8 years ago by
Kitson, why does this patch use path.sep
to generate a module ID for Node.js? CommonJS module IDs and AMD module IDs follow the same semantics (forward-slashes) so I don’t understand why this is being done. Actually I don’t understand why it is any more complicated than id = require.toUrl(normalize(id));
.
comment:23 Changed 8 years ago by
it's using path.sep
because require.toUrl
is generating a path to a file rather than a module id
comment:24 Changed 8 years ago by
While the canonical path separator in Windows is a backslash, Windows APIs are OK with forward-slashes in file paths (it is only the command-line interpreters that are not) and Node.js on Windows is as well. Also the patch only replaces the *first* forward-slash, and it only replaces it on the *output* of require.toUrl
, which seems wrong no matter which way you slice it. I implemented a simpler version of this in dojo2-core similar to what I stated in comment #22 (I think it could actually be as simple as what is in comment #22) and it works fine on Windows.
https://github.com/csnover/dojo2-core/blob/dfc544969d5d7347ba8f9700e2c84f8765400820/node.js#L48-L53 https://github.com/csnover/dojo2-core/blob/dfc544969d5d7347ba8f9700e2c84f8765400820/tests/node.js#L15-L20
Fixes issues loading some node modules