Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

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

13490.patch (55.2 KB) - added by Douglas Hays 8 years ago.
converting calls to dojo.declare to use module return values
13490_2.patch (200.7 KB) - added by Douglas Hays 8 years ago.
added granular dojo dependencies and used module return values
convert.sh (7.9 KB) - added by bill 8 years ago.
helper script to convert files to baseless (script not complete yet)

Download all attachments as: .zip

Change History (23)

Changed 8 years ago by Douglas Hays

Attachment: 13490.patch added

converting calls to dojo.declare to use module return values

comment:1 Changed 8 years ago by Douglas Hays

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 8 years ago by ykami

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 8 years ago by bill

(In [25874]) Convert dijit/_base to use module return values rather than dojo and dijit globals. Gets rid of references to dojo global, and some references to dijit global. Refs #13490 !strict.

comment:4 Changed 8 years ago by bill

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 8 years ago by Douglas Hays

(In [25876]) Refs #13490. Change declare() arguments in dojox/mobile to use module return values. !strict

Changed 8 years ago by Douglas Hays

Attachment: 13490_2.patch added

added granular dojo dependencies and used module return values

comment:6 Changed 8 years ago by Douglas Hays

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 8 years ago by ykami

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 8 years ago by bill

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 8 years ago by bill

Attachment: convert.sh added

helper script to convert files to baseless (script not complete yet)

comment:9 Changed 8 years ago by Douglas Hays

(In [25886]) Refs #13490. Add missing dojo/* dependencies and fully utilize module return values in dojox/mobile/. !strict

comment:10 Changed 8 years ago by ykami

(In [25898]) Refs #13490 !strict Refactored scrollable.js to use module return values. Sorted the deps lists to be less error-prone. Fixed typos in the deps list of _compat.js. Removed unnecessary dependency on dojo.query.

comment:11 Changed 8 years ago by ykami

(In [25912]) Refs #13490 !strict Added dependency on common.js to modules that really rely on common.js functions. Added existence check for this._cv to remove unnecessary dependency on common.js. common.js no longer extends _WidgetBase to add _cv.

comment:12 Changed 8 years ago by Douglas Hays

Resolution: fixed
Status: newclosed

comment:13 Changed 8 years ago by bill

Resolution: fixed
Status: closedreopened

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 8 years ago by ykami

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

In [26080]:

Refs #13490 !strict Removed global references as much as possible. Some of them, such as xhrGet and dojo.hash, cannot be removed yet though. Added all the necessary module dependencies to _compat following Bill's suggestion.

comment:16 Changed 8 years ago by ykami

In [26089]:

Refs #13490 Updated the build profile to reference dijit.registry instead of dijit._base.manager.

comment:17 Changed 8 years ago by ykami

In [26091]:

Refs #13490 !strict Removed most of the references to dojox.mobile. Hope I am doing right..

comment:18 Changed 8 years ago by bill

In [26198]:

[25961] accidentally changed a margin-box to a content-box, although it doesn't seem to make any difference, refs #13490 !strict.

comment:19 Changed 8 years ago by Douglas Hays

Resolution: fixed
Status: reopenedclosed

comment:20 Changed 6 years ago by bill

In [30700]:

reduce uses of dijit variable, refs #13490 !strict

Note: See TracTickets for help on using tickets.