Opened 8 years ago

Closed 7 years ago

#13966 closed enhancement (fixed)

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

Reported by: Brian Carstensen Owned by: bill
Priority: high Milestone: 1.8
Component: DojoX Widgets Version: 1.7.0
Keywords: rotator autorotator Cc: Rawld Gill
Blocked By: Blocking:

Description

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

Attachments (2)

dojox.widget.Rotator-AMD.patch (5.2 KB) - added by Brian Carstensen 8 years ago.
13966_neekfenwick_dojox.widget.Rotator-AMD.patch (65.7 KB) - added by Nick Fenwick 8 years ago.
Updated patch using dojo/on, dojo/aspect, data-dojo-id, and other goodness, and patch direction correct.

Download all attachments as: .zip

Change History (29)

Changed 8 years ago by Brian Carstensen

comment:1 Changed 8 years ago by Brian Carstensen

This should be assigned to cb1kenobi.

comment:2 Changed 8 years ago by cb1kenobi

Owner: changed from dante to cb1kenobi

comment:3 Changed 8 years ago by 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.

comment:4 Changed 8 years ago by bill

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

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.

comment:5 Changed 8 years ago by 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.

comment:6 Changed 8 years ago by bill

Cc: Rawld Gill added

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.

comment:7 Changed 8 years ago by 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?

comment:8 Changed 8 years ago by 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.

comment:9 Changed 8 years ago by bill

Version: 1.7.0

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).

comment:10 Changed 8 years ago by 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.

comment:11 in reply to:  10 ; Changed 8 years ago by 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.

comment:12 in reply to:  11 Changed 8 years ago by 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

comment:13 Changed 8 years ago by bill

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

comment:14 Changed 8 years ago by 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.

comment:15 Changed 8 years ago by 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.

comment:16 Changed 8 years ago by 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.

comment:17 Changed 8 years ago by 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.

comment:18 Changed 8 years ago by 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.

comment:19 Changed 8 years ago by bill

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

Changed 8 years ago by Nick Fenwick

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

comment:20 Changed 8 years ago by Nick Fenwick

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

comment:21 Changed 7 years ago by dylan

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

comment:22 Changed 7 years ago by 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.

comment:23 Changed 7 years ago by Colin Snover

Milestone: 1.82.0

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

comment:24 Changed 7 years ago by 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.

comment:25 Changed 7 years ago by bill

Milestone: 2.01.8

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.

Last edited 7 years ago by bill (previous) (diff)

comment:26 Changed 7 years ago by bill

Owner: changed from cb1kenobi to bill
Status: newassigned

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.

comment:27 Changed 7 years ago by bill

Resolution: fixed
Status: assignedclosed

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

Note: See TracTickets for help on using tickets.