#13490 closed defect (fixed)
change dojox/mobile to reference module return values
Reported by: | Douglas Hays | Owned by: | Douglas Hays |
---|---|---|---|
Priority: | high | Milestone: | 1.7 |
Component: | DojoX Mobile | Version: | 1.7.0b1 |
Keywords: | Cc: | ykami | |
Blocked By: | Blocking: |
Description
Attachments (3)
Change History (23)
Changed 10 years ago by
Attachment: | 13490.patch added |
---|
comment:1 Changed 10 years ago by
Kamiyama-san, please review the attached patch which changes the declare() calls to use module return values. The list of module dependencies for each file will be patched separately.
comment:2 Changed 10 years ago by
Doug, thank you for the patch. It looks ok to me.
Just to be sure, are we going to use declare instead of dojo.declare?
comment:3 Changed 10 years ago by
comment:4 Changed 10 years ago by
Oops, that comment above was for #13494.
Anyway, I looked over the patch too. Besides still using dojo.declare(), it's using the dojo global in lots of other places too, like for dojo.style(), dojo.addClass(), dojo.removeClass(), etc. I'll attach the script that I've been using (still in progress) to help do conversion of all these references.
comment:5 Changed 10 years ago by
Changed 10 years ago by
Attachment: | 13490_2.patch added |
---|
added granular dojo dependencies and used module return values
comment:6 Changed 10 years ago by
Kamiyama-san, the 2nd patch needs to be reviewed and test very closely since it changes a lot. Also, please think about how using module return values will work with mobile's non-dojo code since you replace things like dojo.style, but the code is using domStyle.style. I'm not changing scrollable.js since it is redefining methods on dojo which is going to break baseless conversion.
comment:7 Changed 10 years ago by
Thank you for the 2nd patch. A couple of things I noticed so far.
- dojox/mobile/parser seems to be still accessing the global dojo.
- Some modules receive dojo/_base/kernel as "kernel", the others receive it as "dojo". Should they be consistent?
- _compat seems to be broken. Most of the test cases do not work on FF. Not investigated yet.
I will think about scrollable.js.
comment:8 Changed 10 years ago by
FYI, my intention with renaming "dojo" to "kernel" was that "kernel" would only be used for the symbols defined in kernel.js, specifically experimental() and deprecated(). Then any remaining "dojo" references indicate an incomplete conversion.
Changed 10 years ago by
Attachment: | convert.sh added |
---|
helper script to convert files to baseless (script not complete yet)
comment:9 Changed 10 years ago by
comment:10 Changed 9 years ago by
comment:11 Changed 9 years ago by
comment:12 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:13 Changed 9 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Hi, this conversion looks good but I'm still seeing a few global references:
- dojo.parser.parse (This one might be problematic because of how dojox.mobile.parser overrides the dojo.parser functionality, maybe you intentionally left it? Note though that the current code will break in 2.0 when there is no dojo.parser variable)
- dojo.xhrGet
- dijit.byId, dijit.byNode, etc. (it's referencing dijit as a global variable, should be requiring and then using dijit/_base/manager)
- dojox.mobile.app (again, perhaps this is intentional as that code is superceded by dojox.app?)
- ProgressIndicator.js is accessing dojox.mobile.ProgressIndicator, should access through local variable
- same for dojox.mobile._ScrollableMixin
- reference to dojox.mobile.SwapView should be just SwapView
- compat.js is referencing a bunch of dojox.mobile globals like dojox.mobile.RoundRect. I see how you are trying to, for example, check if dojox.mobile.RoundRect is loaded before patching it, but maybe it's better to load that file all the time. Since _compat.js doesn't even load on phones, it doesn't matter if it pulls in unneeded code. Also, with the AMD loader I don't think you are guaranteed that dojox.mobile.RoundRect loads before compat.js, so there's a race condition.
Also, common.js is loading but not using dijit._WidgetBase, so I think that dependency can be removed.
comment:14 Changed 9 years ago by
Thanks for finding them. I'll take a look. I may not be able to solve all of them, in particular, references to xhr and dojo.hash from dojox.mobile are a bit tricky ones..
comment:19 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
converting calls to dojo.declare to use module return values