Opened 8 years ago

Closed 8 years ago

#13484 closed enhancement (fixed)

dojo/_base/kernel and dojo/_base/config have factoring errors

Reported by: Rawld Gill Owned by: Rawld Gill
Priority: high Milestone: 1.7
Component: Loader Version: 1.7.0b1
Keywords: Cc:
Blocked By: Blocking:

Description

dojo/_base/kernel includes setObject, getObject, exists, _mixin, and mixin. These should be moved to dojo/_base/lang.

dojo/_base/config attempts to sniff for foreign loaders. This is nonsensical since it's impossible to predict how those loaders may define attributes on their script nodes. Instead, the standard should be to use global dojoConfig for foreign loaders.

Further exploration of the config design as implemented between dojo.js, dojo/_base/config, dojo/_base/kernel, and the builder indicates some unnecessary code that can be cut, thereby reducing the size of optimized builds.

Change History (21)

comment:1 Changed 8 years ago by Rawld Gill

Using this ticket to document housecleaning of the loader that accompanied code review executed while writing loader tutorial:

  • made require.signal public; optimized expression of require.on/signal to minify code.
  • made cacheBust boolean instead of forcing the client to choose a unique string
  • removed backcompat modulePaths processing from loader; this is already being done in dojo/_base/kernel
  • made deps and callback work only during bootstrap; after that, just specify deps and callback in normal require arguments...no need to also allow these to be specified as configuration params after bootstrap. This allowed removal of bootstrap function at bottom of implementation.
  • shuffled chunks of code around and added commentary to make the module easier to comprehend

comment:2 Changed 8 years ago by Rawld Gill

(In [25850]) refactored set/getObject, mixin, exists from dojo/_base/kernel to dojo/_base/lang; further loader and kernel cleanup and minifying; refs #13484; !strict

comment:3 Changed 8 years ago by Rawld Gill

(In [25851]) updated dijit to properly consume functions moved from dojo/_base/kernel to dojo/_base/lang in [25850]; refs #13484; !strict

comment:4 Changed 8 years ago by Rawld Gill

(In [25853]) fixed mistake in [25851]; refs #13484; !strict

comment:5 Changed 8 years ago by Rawld Gill

(In [25854]) reverted files that should not have been changed in [25852]; refs #13484; !strict

comment:6 Changed 8 years ago by Rawld Gill

(In [25855]) reverted file that should not have been changed in [25852]; refs #13484; !strict

comment:7 Changed 8 years ago by Rawld Gill

missed by trac,

(In [25852]) updated build app to properly consume functions moved from dojo/_base/kernel to dojo/_base/lang in [25850]; refs #13484; !strict

comment:8 Changed 8 years ago by Rawld Gill

(In [25870]) fixed docs; refs #13484; !strict

comment:9 Changed 8 years ago by Rawld Gill

(In [25872]) removed and replaced local references to public symbols; refs #13484; !strict

comment:10 Changed 8 years ago by Rawld Gill

Status: newaccepted

comment:11 Changed 8 years ago by Rawld Gill

Resolution: fixed
Status: acceptedclosed

comment:12 in reply to:  1 Changed 8 years ago by ben hockey

Replying to rcgill:

  • made cacheBust boolean instead of forcing the client to choose a unique string

just noticed this and the implementation of cacheBust is not going to work with how cacheBust is used in practice. we can't assign arbitrary values to cacheBust because the typical usage of cacheBust is that you include something like a version number of your build as the value for cacheBust in your config. this makes urls like dojo/foo.js?v1234 then when you release your next version, you have dojo/foo.js?v1235. this allows you to set a Cache-Control header with a max-age set to a very large number. when you change the value of the cacheBust config then (and only then) it breaks the cache. ie dojo/foo.js?v1234 is cached "forever" and the way to break it is to use dojo/foo.js?v1235. if we start choosing arbitrary values for the queryString it removes that determinism and breaks the long-lived caching that's intended.

comment:13 Changed 8 years ago by ben hockey

Resolution: fixed
Status: closedreopened

comment:14 Changed 8 years ago by ben hockey

quoting from an oldie but a goodie http://ajaxian.com/archives/dojo-tips-cookies-and-a-nice-curry

djConfig.cacheBust = new Date().valueOf().toString(); // random value. bad in all but dev
djConfig.cacheBust = "version-0.2.1"; // be explicit
dojo.require("dojo.string"); // will get string.js?version-0.2.1

comment:15 Changed 8 years ago by Rawld Gill

Resolution: fixed
Status: reopenedclosed

fixed in [26002]

comment:16 Changed 8 years ago by ben hockey

In [26131]:

refer to lang rather than dojo in dojo/i18n.js. refs #13484 !strict

comment:17 Changed 8 years ago by ben hockey

In [26132]:

fix doc typo from r25870 refs #13484 !strict

comment:18 Changed 8 years ago by ben hockey

Resolution: fixed
Status: closedreopened

in r25850 the following line was added at http://bugs.dojotoolkit.org/browser/dojo/dojo/trunk/_base/lang.js?rev=25850#L613

has("dojo-1x-base") && mixin(dojo, lang);

the has-value for dojo-1x-base is set in the loader and this line stops lang from being mixed into dojo when an alternative loader is used.

define(["dojo"}, function (d) { 
  // with an alternative loader, lang has not been mixed in
});

since dojo/_base/lang.js is the only file that uses this i'm wondering if it was not meant to have been added?

comment:19 Changed 8 years ago by bill

Priority: normalhigh

Marking as priority=high to show up on the 1.7 blocker list. If you really want a flag to control whether dojo is populated with symbols, I guess it should be defined in kernel.js, or maybe it should be reversed, like

if(!has("no-dojo-globals"){ ... }

comment:20 Changed 8 years ago by Rawld Gill

Status: reopenedassigned

comment:21 Changed 8 years ago by Rawld Gill

Resolution: fixed
Status: assignedclosed

In [26612]:

moved has extend-dojo feature to dojo/kernel; refs #13959; fixes #13484; !strict

Note: See TracTickets for help on using tickets.