Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#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)

dojo_node.patch (408 bytes) - added by Kitson Kelly 7 years ago.
Fixes issues loading some node modules
dojo_node.2.patch (693 bytes) - added by Kitson Kelly 7 years ago.
Another patch…
16414.diff (1.4 KB) - added by ben hockey 6 years ago.

Download all attachments as: .zip

Change History (27)

Changed 7 years ago by Kitson Kelly

Attachment: dojo_node.patch added

Fixes issues loading some node modules

comment:1 Changed 7 years ago by Kitson Kelly

Owner: set to Colin Snover
Status: newassigned

comment:2 Changed 7 years ago by Colin Snover

Owner: changed from Colin Snover to Kitson Kelly
Status: assignedpending

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

comment:3 Changed 7 years ago by Kitson Kelly

Status: pendingnew

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 7 years ago by Kitson Kelly

Owner: changed from Kitson Kelly to Colin Snover
Status: newassigned

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 7 years ago by 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.

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

Changed 7 years ago by Kitson Kelly

Attachment: dojo_node.2.patch added

Another patch...

comment:7 Changed 7 years ago by Kitson Kelly

Resolution: fixed
Status: assignedclosed

In [30603]:

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

comment:8 Changed 7 years ago by Kitson Kelly

In [30604]:

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

comment:9 Changed 7 years ago by bill

Milestone: tbd1.8.4

comment:10 Changed 7 years ago by Colin Snover

In [30630]:

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

comment:11 Changed 7 years ago by Colin Snover

In [30631]:

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

comment:12 Changed 7 years ago by Colin Snover

#16159 is a duplicate of this ticket.

comment:13 Changed 6 years ago by ben hockey

Resolution: fixed
Status: closedreopened

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.

comment:14 Changed 6 years ago by ben hockey

i have a possible solution - see the attached patch

Last edited 6 years ago by ben hockey (previous) (diff)

Changed 6 years ago by ben hockey

Attachment: 16414.diff added

comment:15 Changed 6 years ago by 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.

comment:16 Changed 6 years ago by 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.

Last edited 6 years ago by ben hockey (previous) (diff)

comment:17 Changed 6 years ago by Colin Snover

Milestone: 1.8.41.8.5

Moving incomplete issues to next minor release.

comment:18 Changed 6 years ago by Kitson Kelly

#17186 is a duplicate of this ticket.

comment:19 Changed 6 years ago by Kitson Kelly <dojo@…>

Resolution: fixed
Status: reopenedclosed

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? 

comment:20 Changed 6 years ago by 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? 

comment:21 Changed 6 years ago by 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? 

comment:22 Changed 6 years ago by 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));.

Last edited 6 years ago by Colin Snover (previous) (diff)

comment:23 Changed 6 years ago by ben hockey

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

comment:24 Changed 6 years ago by 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

Last edited 6 years ago by Colin Snover (previous) (diff)
Note: See TracTickets for help on using tickets.