Opened 7 years ago

Closed 7 years ago

#16400 closed defect (invalid)

unnecessary reference to module.id in kernel

Reported by: Randy Hudson Owned by: Rawld Gill
Priority: undecided Milestone: 1.9
Component: Loader Version: 1.8.1
Keywords: Cc:
Blocked By: Blocking:

Description

In dojo/_base/kernel.js, there is code:

(require.map && require.map[module.id.match(/[^\/]+/)[0]])

that should be replaced with:

(require.map && require.map["dojo"])

It seems pretty silly to suggest that the dojo kernel module doesn't even know that it is part of the "dojo" package. In general, the notion that a module doesn't know its own ID is pointless since everyone referencing that module clearly knows its ID.

The reference to module.id property was causing a problem when using dojo with an AMD loader because there is no such "id" property documented at http://requirejs.org/docs/api.html

Change History (6)

comment:1 Changed 7 years ago by bill

Component: CoreLoader
Owner: set to Rawld Gill
Status: newassigned

Hmm, the comment above that require.map line says:

a foreign loader or no pacakgeMap results in the above default config

which suggests that it's only used for the dojo loader.

comment:2 Changed 7 years ago by ben hockey

there is no guarantee that dojo will be called "dojo" - one of the features of the loader is that you can remap the package to any name. it's not my call but i'd expect this to be closed and working as designed.

fyi: requirejs supports the id property of a module https://github.com/jrburke/requirejs/blob/2.1.2/require.js#L561-L574

comment:3 in reply to:  2 Changed 7 years ago by Randy Hudson

Replying to neonstalwart:

there is no guarantee that dojo will be called "dojo" - one of the features of the loader is that you can remap the package to any name. it's not my call but i'd expect this to be closed and working as designed.

Mappings are used to map an ID to a path, no? Some loaders then use those paths to individually fetch modules, but the ID of the module is a constant and should always be "dojo/_base/kernel".

fyi: requirejs supports the id property of a module https://github.com/jrburke/requirejs/blob/2.1.2/require.js#L561-L574

I saw that as well, but is require.js an implementation of a specification, or is it the specification? It seems like developers trying to implement an AMD loader suitable for production would be forced to rely on reverse engineering if there's not specification anywhere describing what API a loader needs to provide.

comment:4 in reply to:  1 Changed 7 years ago by Randy Hudson

a foreign loader or no pacakgeMap results in the above default config

which suggests that it's only used for the dojo loader.

You're right, I just found this reference when doing a global search for "module.id". Other references to module.id were causing problems but this one probably wasn't since "require.map" is specific to the dojo loader. Although maybe a has check would be better than testing for require.map.

comment:5 Changed 7 years ago by ben hockey

it seems your argument is heavily based on assuming the module has a constant id - this is not a correct assumption. (let me know if you need proof.)

as for requirejs - you're right - it's certainly not the specification but is an implementation of a specification. module.id is something that hasn't been explicitly specified (https://github.com/amdjs/amdjs-api/wiki) but it is something that has been adopted de facto based on part of the commonjs module specification http://wiki.commonjs.org/wiki/Modules/1.1

The "module" object must have a read-only, don't delete "id" property that is the top-level "id" of the module. The "id" property must be such that require(module.id) will return the exports object from which the module.id originated. (That is to say module.id can be passed to another module, and requiring that must return the original module).

curl, another AMD loader, also has this same feature - https://github.com/cujojs/curl/blob/0.7.2/src/curl.js#L408-L420

it probably should be added to the AMD spec so i've started a thread to discuss this (https://groups.google.com/d/topic/amd-implement/UEDv67PQLv0/discussion).

comment:6 Changed 7 years ago by Rawld Gill

Milestone: tbd1.9
Resolution: invalid
Status: assignedclosed

Thanks for the detailed look randyhudson. But, given all of history, I essentially agree with neonstalwart and don't have much more to add.

Note: See TracTickets for help on using tickets.