Opened 6 years ago

Closed 6 years ago

#16401 closed defect (wontfix)

unnecessary reference to module.id in date/locale

Reported by: Randy Hudson Owned by: Adam Peller
Priority: undecided Milestone: tbd
Component: Date Version: 1.8.1
Keywords: Cc: Rawld Gill
Blocked By: Blocking:

Description

The code:

lang.setObject(module.id.replace(/\//g, "."), exports);

should be replaced with:

lang.setObject("dojo/date/locale", exports);

and:

exports.addCustomFormats(module.id.replace(/\/date\/locale$/, ".cldr"),"gregorian");

with:

exports.addCustomFormats("dojo.cldr"),"gregorian");

In general, the notion that a module doesn't know its own ID is silly since everyone referencing that module must know it.

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 (16)

comment:1 Changed 6 years ago by Adam Peller

see #15678

comment:2 Changed 6 years ago by bill

Cc: Rawld Gill added

In general, the notion that a module doesn't know its own ID is silly since everyone referencing that module must know it.

I wouldn't go that far, since theoretically modules are supposed to be relocatable.

But since the lang.setObject() is just backwards compatibility code I agree the change makes sense, in order to support other loaders for 1.9 (or earlier). #15678 says the same thing.

As for the exports.addCustomFormats() call, I'm not sure what to make of that code... having something called "dojo.cldr" (particularly the "dojo." part) sounds wrong, but I don't know a better way, and I don't understand exactly what its doing.

comment:3 Changed 6 years ago by ben hockey

In general, the notion that a module doesn't know its own ID is silly since everyone referencing that module must know it.

it's actually a best practice that a module does not rely on or know it's own ID. this is why we don't hardcode the ids into the modules. anonymous modules helps us avoid naming collisions between 3rd party packages and allows us to include multiple version of the same package on the same page.

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

also, the lang.setObject call needs to use "." not "/":

lang.setObject("dojo.date.locale", exports);

this line could be safely changed because lang.setObject accounts for the remapping of the package names when it translates to globals.

comment:4 Changed 6 years ago by Randy Hudson

It seems like you are blurring the distinction between a module's ID and the path used by some loaders to individually fetch each modules javascript (the loader we are implementing always fetches every module it needs in a one request). The config map can be used to map a package like "dojo" to the path "dojo1_8", but modules still refer to dojo modules using logical IDs, e.g. "dojo/date/locale". While its path might change, the ID for the locale module is always "dojo/date/locale".

this is why we don't hardcode the ids into the modules

At least within Dijit, that isn't true. You see code like:

return declare("dijit.form.Button", [_FormWidget, _ButtonMixin]…

comment:5 Changed 6 years ago by ben hockey

internally declare uses lang.setObject with that ID to create a global http://bugs.dojotoolkit.org/browser/dojo/tags/release-1.8.1/dojo/_base/declare.js#L820.

lang.setObject then uses a function called getProp http://bugs.dojotoolkit.org/browser/dojo/tags/release-1.8.1/dojo/_base/lang.js#L190 and getProp uses scopeMap to get the globals to be used http://bugs.dojotoolkit.org/browser/dojo/tags/release-1.8.1/dojo/_base/lang.js#L28.

scopeMap is built from packageMap http://bugs.dojotoolkit.org/browser/dojo/tags/release-1.8.1/dojo/_base/kernel.js#L55 and then scopeMap is used to create globals http://bugs.dojotoolkit.org/browser/dojo/tags/release-1.8.1/dojo/_base/kernel.js#L66.

packageMap is the map config (https://github.com/amdjs/amdjs-api/wiki/Common-Config) for the package that kernel belongs to. here you'll see another usage of module.id (http://bugs.dojotoolkit.org/browser/dojo/tags/release-1.8.1/dojo/_base/kernel.js#L49) because you cannot safely assume that the name of the package is "dojo".

it is not safe to assume that the packages provided by dojotoolkit will always have ids of 'dojo', 'dijit' and 'dojox' - they can be whatever the consumer of the package wants them to be.

as an example, consider the following dojoConfig

var dojoConfig = {
    async: 1,
    packages: [
        {
            name: 'fizz',
            location: 'dojo/dojo'
        }
    ]
};

based on that, assuming the location points to the dojo package, the dojo package has an id of "fizz".

comment:6 Changed 6 years ago by Randy Hudson

as an example, consider the following dojoConfig

var dojoConfig = {
    async: 1,
    packages: [
        {
            name: 'fizz',
            location: 'dojo/dojo'
        }
    ]
};

based on that, assuming the location points to the dojo package, the dojo package has an id of "fizz".

I don't think that's correct. Based on that config, the fizz package has a location of "dojo/dojo". The package name and id are the same thing. The dojo package might be configured with some alternate location, but it will always be the "dojo" package. Defining some other package and pointing it at the dojo source doesn't really make sense.

If you search for ".setObject" in dojo, there are around 30 other places where the property id is inlined.

comment:7 Changed 6 years ago by ben hockey

Defining some other package and pointing it at the dojo source doesn't really make sense.

it really could make sense and that's why you can do it. one of the most sensible use cases is supporting 2 versions (or 2 instances of the same version) of dojo on the same page and this is something i deal with as a common use case. one use case to consider is that someone may have used dojo to write a weather widget that you could embed on your page and then if you also have dojo on your page, the author of this weather widget should do the right thing and remap it's version of dojo so as not to interfere or be affected by the one you use.

you really need to do all you can to break this association between the dojo package and the id "dojo" - it is not a guarantee. i've tried my best to show you but it seems i haven't managed to help you see it.

i can write my library with dependencies on "fizz/_base/declare" and use the package config outlined above and if the source for dojo is at dojo/dojo then i am using the dojo package but the id is "fizz". a package does not define it's own id, the id is given to it either implicitly by it's path or explicitly by some other means such as the name of the package in the config. either way, the package never defines it's own id, it is always given an id and so assumptions cannot be made about what that id is.

it's probably hard to see because an overwhelming majority of cases will let the path implicitly define the id of the package but you must believe me when i tell you that the dojo package isn't always going to have the id of "dojo" and hence using module.id is necessary in order to find out the id being used to reference the dojo package.

as for setObject, i've also tried to outline how it can safely use a fixed id because that gets remapped based on changes to the packageMap configuration of the dojo package. again, i haven't managed to successfully convince you with my words - so either you can blindly believe me or you can convince yourself by following through all the code i linked to and seeing how it works.

i'd be happy to keep trying to help you through this so let me know if you have some specific questions i can address otherwise i'll probably just keep talking in circles.

comment:8 Changed 6 years ago by bill

Adam said that the addCustomFormats() API should be removed for 2.0, since it's adding unnecessary API surface (i.e. probably no one is using it).

It looks like the package name registered via addCustomFormats(), i.e. "dojo.cldr", eventually turns into a require("dojo/cldr/...") call in i18n.js. For example, require("dojo/cldr/currency").

Even if module.id is added to the AMD spec, I think we should be using relative paths to load those resources, since that's standard AMD procedure. So it should be "./cldr/currency" rather than "dojo/cldr/currency". But perhaps not easy to change that for 1.x since the code in i18n.js, for backwards-compatibility reasons, translates "." into "/", so that (for example) "dojo.cldr" becomes "dojo/cldr".

comment:9 Changed 6 years ago by ben hockey

fwiw module.id is actually part of the AMD spec but its specified indirectly by referring to the CJS module spec and I hadn't noticed it. james pointed me to the references and will make it a little clearer next week. all the details are in the google groups thread I linked to in #16400.

I think the current usage of module.id within dojo is only to support features needed for backwards compatibility (globals and i18n) which should be able to be removed in 2.0. however, it still stands that a module (and package) can not assume it has a certain id and module.id is the mechanism that provides that information to a module if it should need it.

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

comment:10 Changed 6 years ago by Bryan Forbes

There are actually two issues in play here: package names and how those names map to global variables, and package names and how that changes module.id. We need to look at the first before the second: In the dojo package that is not renamed, all setObject() (and also declare()) calls that have dojo as the first namespace will map that name to the dojo global:

http://bryanforbes.dojotoolkit.org/multi/normal.html (Be sure to have your console open for all of the linked examples)

This is by design for backwards compatibility. However, when you rename the dojo package to fizz, the dojo global stays because no packageMap is set up (what the old scopeMap used to do):

http://bryanforbes.dojotoolkit.org/multi/fizz.html

dojo now has some things, and fizz has some other things that the module.id changes for 1.8 introduced (in the case of the example, fizz has everything from 1.8/dojo/date/locale.js on it and the rest lives on the dojo global). The only way for this to work correctly (have everything in dojo/ live on one global) in 1.8 is with a packageMap:

http://bryanforbes.dojotoolkit.org/multi/packageMap.html

This feels wrong and does not reflect what occurred in 1.7 when a package was renamed without a packageMap:

http://bryanforbes.dojotoolkit.org/multi/fizz17.html

This means that the module.id changes that were made for 1.8 (changes like what are in r29244) when calling setObject() should be backed out in 1.8 because it breaks backwards-compatibility with regard to setting global object correctly without a packageMap.

Now, the actual issue here is the valid use of module.id. In its essence, the module.id used in modules in 1.8 isn't wrong, per-se, because in a pure AMD world with no globals, it would be correct. In fact, a good valid example of correct module.id usage is in dojo/request/script where script elements are created with an ID with a prefix of the module id. It needs to be said that a module ID is the result of taking the package name and concatenating the path to the module within the package. In the case of dojo/request/script, it would be packageName + '/request/script'. The module ID would change depending on what I named the package. As Ben said, this is from the CommonJS spec. I have put together an example showing this using two versions of Dojo and adding the module.id value to the _base/lang module in each version:

http://bryanforbes.dojotoolkit.org/multi/multi.html

As you can see, the module.id changes based on the package name, not based on the location. Could you imagine if two versions of dojo/request/script were being used and the module.id did not change based on package name? They would both look for each others' scripts during requests since they would both use the dojo_request_script prefix for their script creation. This is part of how portable packages work in the AMD and CommonJS world: it should not matter what the package is called. This is also what packageMap is trying to solve for backwards-compatibility.

comment:11 Changed 6 years ago by Randy Hudson

I understand that there is a valid use for a unique module ID in scenarios where multiple implementations of the same module exist, but the fact that this property is called "id" today when "id" is frequently used to mean something else is misleading.

RequireJS also assumes by default that all dependencies are scripts, so it does not expect to see a trailing ".js" suffix on module IDs. RequireJS will automatically add it when translating the module ID to a path.

Pretty much throughout the API documentation, "ID" describes the string you use when requiring a module. If the module "dojo/data/locale" is part of the "dojo" package, and that package is mapped to the path "dojo18", then it would be less confusing to say that the path for that module is "dojo18/date/locale".

I think either the requirejs/AMD documentation needs to be updated (always use module name or module path, and not id), and/or the property on module should be renamed to "path" (or "uuid"?).

comment:12 Changed 6 years ago by ben hockey

at the risk of being offensive i'll be quite direct... the usage of ID within AMD is perfectly in line with how it is frequently used and maybe you just have something awry in your mental model of how AMD module IDs are being used. the part that might be tripping you up is that a particular package will always have a consistent ID within a given context but from one context to another that ID will not remain constant - ie if the dojo package is called "dojo" within one context it will consistently be called "dojo" within that context but the dojo package will not always be called "dojo" in every context that it is being used.

i'm not sure if it will help but if you think of AMD module ids to be a higher layer of abstraction that map down to paths (URIs) and that multiple IDs can potentially map to the same path and there is no guaranteed correlation between the characters that form the ID and the characters that form the path but within a given context a certain ID will always map to the same path then maybe it helps... but i don't know. based on that model, the dojo package (and consequently all of it's modules) can potentially be given any ID and as such the modules within the package cannot make any assumptions about their ID and hence module.id is provided as the way for a module to have knowledge of it's own ID.

comment:13 Changed 6 years ago by Adam Peller

Resolution: wontfix
Status: newclosed

I'd suggest that this discussion is not appropriate for trac. Perhaps move to the mailing list. If somehow this does result in an agreed upon bug, feel free to reopen. The intent is that both of these references will go away in 2.0 anyhow.

comment:14 Changed 6 years ago by ben hockey

Resolution: wontfix
Status: closedreopened

as mentioned by bryan, the lang.setObject call is a bug and should be reverted back to lang.setObject("dojo.date.locale", exports); (see bryan's comment for details) but it's not really related to the original reason this ticket was opened.

comment:15 Changed 6 years ago by Adam Peller

which is exactly why we discourage such discussions on trac. Dojo trac has a TL;DR policy. I'd suggest starting over with a new ticket.

comment:16 Changed 6 years ago by Adam Peller

Resolution: wontfix
Status: reopenedclosed

or consider reopening #15678 instead

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