[patch] [ccla] dojo.cache causes extra requests with non-dojo build systems like RequireJS

[Colin Snover]

Related to #11869.

Currently, dojo’s AMD definitions are defined with all the proper text! requirements for templates, but when building with a different AMD-compatible build system (like RequireJS), these required strings are never used and end up being needlessly re-requested by dojo.cache. There are a couple of potential ways of dealing with this that I can think of off the top of my head:

1. Update dojo.cache to try to retrieve text using require('text!' + module + '/' + url) before falling back to the normal loader.
2. Update every templated widget to assign the text to a variable and then change templateString lines to templateString: tmpl || dojo.cache(…).

Both work, #1 is obviously much quicker and easier and touches less stuff but assumes that the loader follows the optional part of the AMD specification regarding dependency scanning. I’ve confirmed that it does at least work with RequireJS; here’s a potential patch:

--- dojo-release-1.6.0-src/dojo/cache.js.orig	2011-02-21 08:11:57.000000000 -0600
+++ dojo-release-1.6.0-src/dojo/cache.js	2011-03-04 16:11:52.000000000 -0600
@@ -82,7 +82,7 @@
 		}else if(val === null){
 			//Remove cached value
 			delete cache[key];
-		}else{
+		}else if(typeof require === "undefined" || !(val = require("text!" + module + '/' + url))){
 			//Allow cache values to be empty strings. If key property does
 			//not exist, fetch it.
 			if(!(key in cache)){

[bill]

Seems like we should have done #2 in 1.6, but too late now. (Could be put into a 1.6.1, although it's not a critical bug fix.)

For 1.7, when we have a real AMD loader, seems like the best option is #3, to get rid of the dojo.cache() calls altogether.

In any case, assigning to kzyp since he's the one that added those duplicate references to begin with, in [23143]. Or was that a checkin on rcgill's behalf?

[Colin Snover]

I found at least one file that’s missing a text! definition, dijit/Tooltip.js. It turns out that the proposed patch #1 falls over and throws an exception in this case instead of just returning nothing, which makes sense. I’ll try poring over dijits and open a separate ticket to address that.

[dante]

see also #12409 ... same issue reported basically, different patch.

[Adam Peller]

suggested patch from Chuck Dumont (IBM, CCLA)

[dante]

I like csnover's shorthand version (needs the .replace() on . vs / though), but this brings up more issues wrt to the text! usage. do we want to go #2 and fix this for 1.7, 16.1 or what?

[chuckd]

requirejs throws if the requested resource is not found unless you replace the onError method, so the call to require really needs to be in a try clause.

[Colin Snover]

If someone’s using an AMD loader and they haven’t registered the dependency in the define call, it’s not entirely unreasonable to think that it should throw an exception since that would indicate an error in programming, is it?

[ben hockey]

i don't think #2 is an option in 1.6 due to the build transform that happens to support the previous build. consider this incomplete snippet

before the build transform:

dojo.define(['text!templateString', ...], function (templateString, ...) {
    dojo.declare([ ... ], {
        templateString: templateString || dojo.cache('templateString'),
        ...
    });
});

after the build transform to remove AMD:

   dojo.declare([ ... ], {
        templateString: templateString || dojo.cache('templateString'),    // ReferenceError: templateString is not defined
        ...
   });

in 1.7, we could stop using dojo.cache and convert dijit to only reference the templateString from the text plugin so that we don't use dojo.cache internally. this would be best practice anyway.

in doing this conversion as a 2-phased process, some things were left out (overlooked) in the first phase. in 1.6, is it too harsh for this to be one of those things that is the reason why AMD is not fully supported until 1.7?

[bill]

Yah, sounds like you should leave 1.6 alone and just fix 1.7.

[Rawld Gill]

I think we should leave this alone:

• fix 1 assumes the plugin will remain "text!". It is likely to change to "dojo/text!"
• fix 2 breaks the 1.6 build (as pointed out by neonstalwart)

For 1.6, everything still works, albeit not optimally. But for an officially unsupported feature, this seems enough.

I am addressing this currently in 1.7 and will post a xref to the fix to this ticket.

[chuckd]

Replying to neonstalwart:

i don't think #2 is an option in 1.6 due to the build transform that happens to support the previous build. consider this incomplete snippet

before the build transform:

dojo.define(['text!templateString', ...], function (templateString, ...) {
    dojo.declare([ ... ], {
        templateString: templateString || dojo.cache('templateString'),
        ...
    });
});

after the build transform to remove AMD:

   dojo.declare([ ... ], {
        templateString: templateString || dojo.cache('templateString'),    // ReferenceError: templateString is not defined
        ...
   });

in 1.7, we could stop using dojo.cache and convert dijit to only reference the templateString from the text plugin so that we don't use dojo.cache internally. this would be best practice anyway.

in doing this conversion as a 2-phased process, some things were left out (overlooked) in the first phase. in 1.6, is it too harsh for this to be one of those things that is the reason why AMD is not fully supported until 1.7?

Not sure I follow your example. The inlined define of the template string is an artifact of the requirejs build optimizer. Why would any of these be in the source code that is being transformed to remove AMD?

[ben hockey]

Not sure I follow your example. The inlined define of the template string is an artifact of the requirejs build optimizer. Why would any of these be in the source code that is being transformed to remove AMD?

i'll try to explain in as few words as possible but i usually take more than needed :)

to clarify, nothing in my snippets are related to requirejs or any other AMD optimizer - this is only related to the build tool in dojo 1.6.

for backwards compatibility, in 1.6, our build tool transforms AMD code back to dojo.provide/dojo.require code and then runs the build as it was in 1.5. this is done so that existing user dojo.provide/dojo.require code can co-exist with dojo 1.6 AMD code.

we MUST write dojo/dijit library code to work with the backwards compatible transform and because of this we are not able to completely leverage what AMD can provide.

in my example, the build transform i refer to is this backwards compatible transform - it is unrelated to requirejs or any other AMD build tool. after the transform and the build, there is no more reference to define in any of the code. for this reason, none of the code inside the AMD "factory" can rely on referencing the arguments passed into the factory because the factory no longer exists after the transform. this is why trying to reference templateString || dojo.cache('templateString'), will cause a ReferenceError?.

in converting to AMD, we came across a number of similar issues that we worked around in various ways. however, since we have not yet produced a new build tool or converted our tests to this format, we cannot claim with any certainty to officially support AMD. it's good to find out about these cases that have not been accounted for in 1.6 so that they are not overlooked in 1.7, but until we have tests and a build tool that adequately support the format then there will be shortcomings.

1.6 is a transitional release wrt AMD and in my opinion we should only patch anything that actually broke backwards compatibility. for 1.7 we will work to ensure that dojo is complete wrt AMD. i think any effort on 1.6 in this respect is going to take away from time that could be spent moving 1.7 further along.

[chuckd]

Replying to neonstalwart: OK, I see what you're saying. Thanks for clarifying. I agree that #2 is not a solution for 1.6. But what's wrong with #1 as a temporary solution in 1.6 until it can be fixed the right way in 1.7?

[ben hockey]

Replying to chuckd:

Replying to neonstalwart: OK, I see what you're saying. Thanks for clarifying. I agree that #2 is not a solution for 1.6. But what's wrong with #1 as a temporary solution in 1.6 until it can be fixed the right way in 1.7?

i'm not sure it's worth the effort for 1.6 but since rawld took this ticket for himself, i'll let him decide what he'll do with it but here's what i think would be my preferred solution. put the responsibility back on dojo's text plugin and implement a write function (or a pluginBuilder property) that inlines the text resource and also registers the inlined resource with dojo.cache. it would produce output something like:

define('text!url', ['dojo/cache'], function (cache) {
    var value = "inline text";
    cache({toString:function(){return 'url';}}, value);
    return value;
});

this output is similar to what is produced by the requirejs text plugin with the addition of the call to register the inlined resource with dojo.cache

if you're using dojo with requirejs then you *should* be using dojo's plugins so it makes sense to put this code in that plugin. also we won't need to add any AMD specific code to somewhere other than code that is already AMD related.

[chuckd]

Thanks Ben. Interesting proposal. That sounds like it would work.

Just to clarify, I'm not trying to push for fixing this in 1.6. My questions were motivated by curiosity about the concerns with the proposed fixes. I think it didn't come across that way. Anyway, thanks for taking the time to explain.

[ben hockey]

ok, i guess i didn't really give my concerns for #1 but it comes back to restricting AMD specific shims to AMD related code/files where possible.

[Rawld Gill]

Ben's suggested fix is already implemented and tested; see...

​https://github.com/dojo/dojo/blob/master/lib/plugins/text.js#L59 + ​https://github.com/dojo/dojo/blob/master/lib/plugins/text.js#L18

and

​https://github.com/altoviso/bdBuild/blob/master/lib/plugins/text.js#L22

I'm in the process of rolling all of this (bdBuild, a version of bdLoad that can fall back into sync dojo.require/provide, and a new bootstrap) into trunk.

Does everybody agree to leave 1.6 alone and press forward with 1.7?

[dojo_master]

Hey guys,

I have a very extensive Dojo app, and I am on dojo 1.4.

In firefox 3.x, at random times, my page stops loading on cache.js (this is the last script import I see in firebug. Could this defect cause this? If so, how can I resolve with an update to cache.js in dojo 1.4?

[ben hockey]

Replying to dojo_master:

Hey guys,

I have a very extensive Dojo app, and I am on dojo 1.4.

In firefox 3.x, at random times, my page stops loading on cache.js (this is the last script import I see in firebug. Could this defect cause this? If so, how can I resolve with an update to cache.js in dojo 1.4?

this ticket is related to changes made during the development of 1.6. it has no relevance to 1.4 at all so your problem is not related to this.

[Rawld Gill]

As of v1.7, the approved method of interning strings is by including the string resource as a dojo/text! resource in the module's dependencies. For example, see all dijit templates.

For legacy v1.x code, the build application should attempt to intern text resources referenced with dojo.cache. This enhancement is now being tracked at #13050.

It is unrealistic to expect third-party loaders and builders to properly handle dojo.cache. Client code that is going to make any changes at all should simply use an appropriate AMD plugin (e.g., dojo/text!).

Lastly, as of v1.7, the functionality if dojo.cache is contained within the dojo/text! plugin.

[skaegi]

In Eclipse/Orion? we're using Dojo 1.6.1 with RequireJS more or less successfully. To optimize we did have to patch dojo.cache (as per #12409) and did a similar text! fix to dijit.Tooltip (as per #12387). I recognize there might be other issues however these were the only two changes needed to get an optimized build going for us.

If possible it would be really helpful to us and help for AMD adoption in our community if this change was done on HEAD in the 1.6 stream and better still in a 1.6.2.