[patch] [cla] dojox.widget.Rotator conversion to AMD format

[Brian Carstensen]

Simple conversion of dojox.widget.Rotator, AutoRotator?, the rotator controllers, and transitions to AMD format.

[Brian Carstensen]

This should be assigned to cb1kenobi.

[Nick Fenwick]

I just wasted several hours trying to convert Rotator, being an AMD newbie, then ran into this patch. It's 5 months old, can this please be targeted at 1.7.x? It could easily have made it into 1.7.2.

[bill]

Out of curiosity, why do you care if it's AMD format or not? The current format presumably will work fine until 2.0.

I also note that the conversion here is not complete; it's not using minimal dependencies but rather the whole dojo object.

[Nick Fenwick]

By the way, also need to change dojox.profile.js to remove AutoRotator?, Rotator and rotator from the excludes regex, otherwise the build complains that they're AMD but not tagged as AMD.

I agree that it seems a really minimal conversion. Perhaps I should provide a new patch that uses the much more specific module requires that I cooked up? I can see from this simple patch what I was doing wrong (I tried to use declare() and got completely tied up in where the various pieces of dojox.widget.rotator.* should be put).

I care if it's AMD because of a warning on http://dojotoolkit.org/reference-guide/dojo/domReady.html#sync-loader saying:

"You should not use dojo/domReady! in any modules that may be loaded with the legacy synchronous loader.

In other words, if your application does not specify async:true as a data-dojo-config parameter, or if it loads modules via dojo.require() instead of the new AMD require() API, then using dojo/domReady! may cause dojo.ready() to call it’s callback before all the modules have loaded."

I'm trying to convert my project to use "async:true", and am quite confused as to whether it's wise or safe to mix async:true and any modules that use the old dojo.provide/dojo.declare syntax. I ran into trouble relating to bug #14808, had to patch loader.js to fix some dojo/ready problem I ran into when the ready event didn't fire. It's left me rather paranoid :) So I'm presuming for now that we must use all AMD or nothing.

[bill]

I see, thanks for the explanation. I think it's OK to mix-and-match in theory, although I wouldn't be surprised if there were bugs.

The warning message sounds like no dojo modules should use dojo/domReady! since any dojo module may be loaded via an app still using the dojo.require() syntax. Different from your issue.

Anyway, if you have a better patch and a CLA that would be good.

[Nick Fenwick]

I have my reservations about the patch I just submitted:

• there's a "dojo.fx.easing = dojo.fx.easing || easing;" line that I borrowed from the original patch and couldn't work around. It feels horrible. It's so that e.g. transitionParams="easing: dojo.fx.easing.cubicInOut" can be evaluated as json.
• I set selectorEngine: 'acme' in the data-dojo-config of the tests files, because the lite engine crashed. Something to do with querySelectorAll .. see ​http://imageshack.us/photo/my-images/513/domexception12.jpg/

I have a CLA on file. Can someone vet this patch please?

[Nick Fenwick]

Just remembered my third reservation about my patch:

• had to specify dijit name in declare, e.g. declare("dojox.widget.Rotator", null, { ... }) .. I thought you could leave the dijit name out, i.e. declare(null, { ... }) and the AMD system would figure out the dijit name of dojox.widget.Rotator from the file it's in, but I found the tests files failed as the parser could not resolve the dijit name dojox.widget.Rotator.

[bill]

Generally we've been leaving in the global names (your e.g. declare("dojox.widget.Rotator", null, { ... }) example) until 2.0, so that the dojox.widget.Rotator global variable gets created, for backwards compatibility. But eventually (or better yet, now) the test files should be updated to say data-dojo-type="dojox/widget/Rotator" thus not referencing the global.

I was hoping Chris would handle your patch since the ticket is assigned to him (and presumably Rotator is his code).

[Nick Fenwick]

Ah I see. If I change the Rotator.js and others to omit their specific name, won't that break back compat now, for everyone (like the tests file) that use data-dojo-type="dojox.widget.Rotator"? Sounds bad to me. Probably a change that should wait for 2.0.

I tried changing a tests file so it uses dojox/widget/Rotator, but it errors with "Could not load class 'dojox/widget/Rotator'". Boo.

[bill]

Replying to neek:

Ah I see. If I change the Rotator.js and others to omit their specific name, won't that break back compat now, for everyone (like the tests file) that use data-dojo-type="dojox.widget.Rotator"? Sounds bad to me. Probably a change that should wait for 2.0.

Right.

I tried changing a tests file so it uses dojox/widget/Rotator, but it errors with "Could not load class 'dojox/widget/Rotator'". Boo.

That indicates that you don't have a file called dojox/widget/Rotator.js that's returning the Rotator class.

[Nick Fenwick]

Replying to bill:

That indicates that you don't have a file called dojox/widget/Rotator.js that's returning the Rotator class.

Yes, I do, it's dojox/widget/Rotator.js, and it's pulled in by require(["dojox/widget/Rotator" ...]) .. am I missing something very obvious in your comment? I seem to be misunderstanding _something_, I haven't been able to get the slash notation to work in parsed markup in another project either, despite following what seems to be a very simple approach, for example this page googled at random has examples that use data-dojo-type="dijit/form/FilteringSelect": ​http://livedocs.dojotoolkit.org/dijit/form/FilteringSelect#known-issues

[bill]

Probably just you are using 1.7, and the new data-dojo-type="dojox/widget/Rotator" syntax support is only in trunk .

[Nick Fenwick]

I think I've finished with this patch, by the way.

I'm wondering if html.style() is correct, or if we should now use dojo/dom-style and domStyle.set() .. also, html.destroy(), or dojo/dom-construct and domConstruct.destroy()?

If someone could review the patch and at least advise where it could be improved, I'd be happy to amend it to fit the preferred methodology.

[bill]

It should be domStyle.set() and domConstruct.destroy(), see ​http://livedocs.dojotoolkit.org/releasenotes/migration-2.0 for general notes on this type of thing.

Another thing I noticed is that there's still a reference to dojo.Animation in Wipe.js.

Oh, and the code below is wrong because after the conversion to use dojo/topic, it shouldn't have brackets (around type, this, params):

topic.publish(this.id + "/rotator/update", [type, this, params || {}]);

For extra credit you could convert the calls to connect.connect() to be calls to on(), remove the "on" prefix from event names, and also change data-dojo-type="a.b.c" to data-dojo-type="a/b/c". But even without that it's a big improvement.

[Nick Fenwick]

Great, that's exactly what I needed to hear. I'm a bit confused between dojo/_base/connect and dojo/on. It seems dojo/on is the one to prefer.

I guess when you say to change to data-dojo-type="a/b/c" that this will be merged and used along with the current changes on trunk. I can't actually use a/b/c in my local environment because I'm on 1.7.2 which doesn't have that support yet. I'll fetch trunk and do my further dev against that.

Will get to this when I can, been working flat out today.

[bill]

Thanks. You are right that dojo/on is the one to prefer, and that data-dojo-type="a/b/c" is a 1.8 (aka trunk) feature.

[Nick Fenwick]

There is a little cruft involving the dojo.fx namespace that seems to be to do with naming easing functions by name, e.g. transition: "dojo.fx.easing.groovy" .. i'd rather someone who knows what's happening with the way the AMD declare() system works try to sort that out. Probably a problem that will wait for a future release?

Otherwise this patch is ready to go again.

[bill]

Looks like the patch is backwards? The green looks like the pre-AMD version.

[Nick Fenwick]

Updated patch using dojo/on, dojo/aspect, data-dojo-id, and other goodness, and patch direction correct.

[Nick Fenwick]

Shazbot, you're right, I diffed in the wrong direction. Correction now attached.

[dylan]

dojox/widget/rotator/Controller.js still have dojo/_base/connect. Were those intentional?

[Nick Fenwick]

No, that's my ignorance of the new AMD style APIs. If something needs brushing up to bring it into line, please do so.

[Colin Snover]

1.8 is frozen. Move all enhancements to next release. If you need an exemption from the freeze for this ticket, contact me immediately.

[Colin Snover]

Assuming all unit tests (if any) continue to work with the patch, this is approved to land on trunk _before_ 1.8b1, i.e. before Friday. It sounds like there is some extra work that needs to be done so please Refs instead of Fixes this ticket in the commit.

[bill]

Judging from Dylan's checkins, apparently we are still doing AMD conversions during beta, so I'll try to check this in. It's got a few errors that need to be fixed such as:

• return values of rotator/* modules are weird
• on.once() is only for DOMNodes, not for connecting to regular functions
• I don't understand why the line dojo.fx.easing = dojo.fx.easing || easing is needed at all

The dojo/_base/connect references need to be updated for 2.0 but strictly speaking they aren't part of AMD conversion.

[bill]

Brian and neek, thanks for the patches.

Neek - about the dojo.fx.easing thing, if the test file (or user application) wants to access dojo.fx.easing as a global variable, then it has to set it itself. That doesn't need to be done in the Rotator widget.

[bill]

In [29287]:

AMD conversion for Rotator/AutoRotator. Rough edges include:

• Still using global variables to access the transitions. They should be specified as MIDs or MID/property combos, ex: "dojox/widget/Slide.slideDown".
  • Some code using deprecated APIs like dojo.connect()... but this is a separate concern from AMD.

Fixes #13966 !strict