Opened 8 years ago
Closed 8 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 8 years ago by
comment:2 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
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.
comment:10 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
Resolution: | → wontfix |
---|---|
Status: | new → closed |
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 8 years ago by
Resolution: | wontfix |
---|---|
Status: | closed → reopened |
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 8 years ago by
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 8 years ago by
Resolution: | → wontfix |
---|---|
Status: | reopened → closed |
or consider reopening #15678 instead
see #15678