dojo/node has issues with loading some modules

[Kitson Kelly]

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.

[Kitson Kelly]

Fixes issues loading some node modules

[Colin Snover]

If the modules support AMD shouldn’t you be loading them directly?

[Kitson Kelly]

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.

[Kitson Kelly]

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.

[Colin Snover]

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.

[Kitson Kelly]

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.

[Kitson Kelly]

Another patch...

[Kitson Kelly]

In [30603]:

dojo/node can now load CommonJS modules dependent on modules that try to detect AMD, fixes #16414

[Kitson Kelly]

In [30604]:

dojo/node can now load CommonJS modules dependent on modules that try to detect AMD, refs #16414

[Colin Snover]

In [30630]:

Fix incorrect non-portable reference to dojo package. Refs #16414.

[Colin Snover]

In [30631]:

Fix incorrect non-portable reference to dojo package. Refs #16414. 1.8 backport

[Colin Snover]

#16159 is a duplicate of this ticket.

[ben hockey]

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.

[ben hockey]

i have a possible solution - see the attached patch

[Kitson Kelly]

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.

[ben hockey]

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.

[Colin Snover]

Moving incomplete issues to next minor release.

[Kitson Kelly]

#17186 is a duplicate of this ticket.

[Kitson Kelly <dojo@…>]

In ddb069635e9577560db37f42025be93e1ea19f09/dojo:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

[Kitson Kelly <dojo@…>]

In 5477c6baefa87acb945233a4c765ebb26d4d37aa/dojo:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

[Kitson Kelly <dojo@…>]

In 4725ce63d3b63eddb212edd2c4f4fc167884e3b2/dojo:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

[Colin Snover]

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));.

[ben hockey]

it's using path.sep because require.toUrl is generating a path to a file rather than a module id

[Colin Snover]

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