Opened 11 years ago
Closed 10 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)
Change History (29)
Changed 11 years ago by
Attachment: | dojox.widget.Rotator-AMD.patch added |
---|
comment:1 Changed 11 years ago by
comment:2 Changed 11 years ago by
Owner: | changed from dante to cb1kenobi |
---|
comment:3 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
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 ofdojox.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 10 years ago by
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 follow-up: 11 Changed 10 years ago by
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 follow-up: 12 Changed 10 years ago by
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 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
Looks like the patch is backwards? The green looks like the pre-AMD version.
Changed 10 years ago by
Attachment: | 13966_neekfenwick_dojox.widget.Rotator-AMD.patch added |
---|
Updated patch using dojo/on, dojo/aspect, data-dojo-id, and other goodness, and patch direction correct.
comment:20 Changed 10 years ago by
Shazbot, you're right, I diffed in the wrong direction. Correction now attached.
comment:21 Changed 10 years ago by
dojox/widget/rotator/Controller.js still have dojo/_base/connect. Were those intentional?
comment:22 Changed 10 years ago by
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 10 years ago by
Milestone: | 1.8 → 2.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 10 years ago by
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 10 years ago by
Milestone: | 2.0 → 1.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.
comment:26 Changed 10 years ago by
Owner: | changed from cb1kenobi to bill |
---|---|
Status: | new → assigned |
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.
This should be assigned to cb1kenobi.