Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#12844 closed enhancement (fixed)

[patch] [cla] Update gfx, charting, gauges to AMD using "baseless" minimal dependencies

Reported by: Chris Mitchell Owned by: Chris Mitchell
Priority: high Milestone: 1.8
Component: DojoX GFX Version: 1.6.0
Keywords: 1.7-mobile Cc: ttrenka@…, Eugene Lazutkin, pruzand@…, etissandier@…, cjolif@…, durocher.marc@…, eaullas+pro@…
Blocked By: Blocking:

Description (last modified by Chris Mitchell)

To reduce mobile dependency/footprint reduction and to allow for async loading benefits (also helps desktop), we need to convert the following packages to use AMD with "baseless" dependencies (see recent dojox.mobile refactoring for good examples).

Here's the plan to divide and conquer (by this Friday!):

dojox.gfx - Eugene
, Patrick(platform test/regression assist)

dojox.charting - Christophe
(Tom will work on non-gfx related dojox modules that still need AMD migration)

dojox.gauges - Emmanuel

dojox.geo.charting - Erwan

dojox.geo.openlayers - Marc

Attach patches with the above file names to this ticket when ready. We'll prob need to do this layered, starting with dojox.gfx, then charting/gauges, then geo.

Change History (94)

comment:1 Changed 9 years ago by Chris Mitchell

Cc: eaullas+pro@… added; eaullas+pro@… removed

comment:2 Changed 9 years ago by Chris Mitchell

Cc: ttrenka@… added

comment:3 Changed 9 years ago by Chris Mitchell

Description: modified (diff)

comment:4 Changed 9 years ago by Chris Mitchell

Description: modified (diff)

comment:5 Changed 9 years ago by Chris Mitchell

Description: modified (diff)

comment:6 Changed 9 years ago by Chris Mitchell

Description: modified (diff)

comment:7 Changed 9 years ago by Eugene Lazutkin

Cc: Eugene Lazutkin added; eugene@… removed
Milestone: tbd1.7

comment:8 Changed 9 years ago by Chris Mitchell

Please keep and note a list of the dijit framework classes needed by each package as you go, so we can focus on those for amd baseless.

comment:9 Changed 9 years ago by Chris Mitchell

Description: modified (diff)
Status: newassigned

comment:10 Changed 9 years ago by bill

Guys, be careful not to follow the dojox.mobile example too closely. Specifically, don't change the dojo.declare() lines because that will break the API doc parser, specifically it won't be able to trace inheritance. At least, that's my impression. (I asked Dustin to roll back his changes to dojo.declare() in dojox.mobile.)

comment:11 Changed 9 years ago by Chris Mitchell

will discuss the declare issue with rawld. the code is actually done the way we want to do it for 1.8/2.0 right now, and the reason stated to back out the changes is that rawld cant fix the api doc tool in time for ga (1.5 months from now). Rather than rolling back a ton of changes that we know work and are inline with future, we can offer to help rawld fix the api doc tool. -Chris

comment:12 Changed 9 years ago by Chris Mitchell

On the irc meeting tonight, we decided that we're not going to address the api-doc tool issues at this time. So as you port these modules modules to AMD, DO only use coding style 1 below--DON'T use style 2:

Style 1: dojo.declare("dojox.mobile.ToggleButton?", [dojox.mobile.Button, dijit.form._ToggleButtonMixin] ...

This is how things normally are already (before u start porting to amd)


Style 2: dojo.declare("dojox.mobile.TextBox?",[WidgetBase?,FormWidgetMixin?,TextBoxMixin?]

The option 2 style declaration is what you'll find inside of the define() in dojox.mobile currently.
This way breaks the api-doc parser in 1.7.
Dustin will be backing out the declare() function calls in dojox.mobile to be how they were previously.

comment:13 Changed 9 years ago by Chris Mitchell

Correcting formatting... On the irc meeting tonight, we decided that we're not going to address the api-doc tool issues at this time. So as you port these modules modules to AMD, DO only use coding style 1 below--DON'T use style 2:

Style 1: dojo.declare("dojox.mobile.ToggleButton?", [dojox.mobile.Button, dijit.form._ToggleButtonMixin] ... This is how things normally are already (before u start porting to amd)

Style 2: dojo.declare("dojox.mobile.TextBox?",[WidgetBase?,FormWidgetMixin?,TextBoxMixin?] The option 2 style declaration is what you'll find inside of the define() in dojox.mobile currently. This way breaks the api-doc parser in 1.7. Dustin will be backing out the declare() function calls in dojox.mobile to be how they were previously.

comment:14 Changed 9 years ago by Chris Mitchell

disregard the question marks...something happened with the wiki formatting.

comment:15 Changed 9 years ago by Eugene Lazutkin

Probably I missed the point: what is the relationship between dojo.declare(), which is an OOP helper, and AMD, which loads modules using older dojo.require() and dojo.provide() and the newer module wrapper? I was under impression that these are orthogonal concepts.

comment:16 Changed 9 years ago by Chris Mitchell

they are orthogonal issues, the doc parser fails on the declare using style 2 because it cant resolve the extends list variable names to classes. This happened because when modules were converted to amd baseless, in the defines args list, we used the shortened variable names, rather than the full object path classnames, and we changed declare to match those shorter arg names. For the api doc tool to resolve the vars to actual declared classes, it would need to map the short arg names to the module paths in the define, resolve using amd loader resolution rules and the look for the declared classes in those modules (or have a manual documentation metadata added to allow a dev to specify the mapping manually)

comment:17 Changed 9 years ago by tissandier

I have attached my current work on AMD for gauges. I think is is complete.

comment:18 Changed 9 years ago by cjolif

I have attached a version of dojox.charting AMD, in addition to the comments in the upload, please note that I did left unchanged the indentation of classes even when they should have been indented as part of the define function. I did that to help reviewing the patch by concentrating on actual changes not indentation changes. A second pass can be done afterward to fix indentation.

Also all of this should probably be re-tested when gfx and other dojox module used will be passed to AMD.

Finally a last comment it seems to me that dijit is importing the full dojo base (dojo), and so I think all the work around baseless is a bit useless as in a lot of cases dijit will be used and require smaller modules won't be very useful? Anyway that is done now.

comment:19 Changed 9 years ago by bill

Summary: Update gfx, charting, gauges to AMD using "baseless" minimal dependencies[patch] [cla] Update gfx, charting, gauges to AMD using "baseless" minimal dependencies

Yah, dijit needs work, Menu and CalendarLite are streamlined but nothing else yet... are you referring to dijit.Tooltip? I know charting uses dijit's tooltip, I don't know anything else that it's using.

comment:20 Changed 9 years ago by cjolif

chrism if you use widgets in dojox/charting/widget you get _Widget and what comes with it. But I agree you can live without them this is just they are super handy to create your chart instead of using JS code.

comment:21 in reply to:  20 ; Changed 9 years ago by Chris Mitchell

Replying to cjolif:

chrism if you use widgets in dojox/charting/widget you get _Widget and what comes with it. But I agree you can live without them this is just they are super handy to create your chart instead of using JS code.

consider using dijit._WidgetBase, and pulling in explicit minimal dependencies if possible.

comment:22 in reply to:  21 ; Changed 9 years ago by cjolif

Replying to chrism:

Replying to cjolif:

chrism if you use widgets in dojox/charting/widget you get _Widget and what comes with it. But I agree you can live without them this is just they are super handy to create your chart instead of using JS code.

consider using dijit._WidgetBase, and pulling in explicit minimal dependencies if possible.

Yes I already considered this, but to minimize the risk I would prefer to see AMD committed and then work on reducing even further to _WidgetBase as here this in not just AMD work this changing class hierarchy?

comment:23 in reply to:  18 Changed 9 years ago by Chris Mitchell

Replying to cjolif:

Finally a last comment it seems to me that dijit is importing the full dojo base (dojo), and so I think all the work around baseless is a bit useless as in a lot of cases dijit will be used and require smaller modules won't be very useful? Anyway that is done now.

As part of the mobile effort, which required separating desktop/mobile subclasses between dijit & mobile, there are some framework portions of dijit that have been refactored to not pull in all of dojo. For example see Slider and TextBox??'s dependency chains. The abstract base class which is common between the desktop/mobile variants should be dependent on a lighter-weight dijit._WidgetBase and base-less, minimal, specific dependencies.

We should try to take the same approach with gfx based widgets which do not pull in heavier dijits. Eg. DataChart??, or ChartWidget?? as we refactor, so that when these widgets are used in mobile scenarios they have lighter dependency chains. In 1.8, an additional pass needs to be taken through the remainder of dijit to further refactor on the desktop widgets.

comment:24 in reply to:  22 Changed 9 years ago by Chris Mitchell

Replying to cjolif:

(re: refactoring to depend only on dijit._WidgetBase) Yes I already considered this, but to minimize the risk I would prefer to see AMD committed and then work on reducing even further to _WidgetBase as here this in not just AMD work this changing class hierarchy?

Makes sense, we can do an initial pass to get on AMD "baseless" (except for dijit dependencies), and another pass to focus in on remaining dijit dependencies.

comment:25 Changed 9 years ago by tissandier

A new version of gauges, with better baseless support...

comment:26 Changed 9 years ago by Chris Mitchell

I've verified that all gfx testcases except resizeTest work on canvas and svg renderers.
PatrickR: Can you please test now on VML and Silverlight, various IE's
Please verify the above patches, now that gfx core is in place. I tried various charting tests and there appear to be problems.
Thanks, Chris

comment:27 Changed 9 years ago by Chris Mitchell

r24798 is for gfx updated to asynch AMD support

comment:28 Changed 9 years ago by Chris Mitchell

(In [24799]) refs #12844 gfx port to async amd !strict

comment:29 Changed 9 years ago by Chris Mitchell

from pruzand this morning: I run the vml and silverlight tests, everything is ok.
pruzand now testing on IE6/7

comment:30 Changed 9 years ago by Chris Mitchell

(In [24807]) refs #12844 fix all known regressions in gfx amd port !strict

comment:31 Changed 9 years ago by Chris Mitchell

(In [24808]) refs #12844 port charting to AMD !strict

comment:32 Changed 9 years ago by Chris Mitchell

(In [24809]) refs #12844 port charting to AMD !strict

comment:33 Changed 9 years ago by Chris Mitchell

(In [24812]) refs #12844 converted geo charting to AMD

comment:34 Changed 9 years ago by Chris Mitchell

(In [24813]) refs #12844 convert dojox/geo/openlayers and dojox/gauges to AMD format !strict

comment:35 Changed 9 years ago by Chris Mitchell

Resolution: fixed
Status: assignedclosed

comment:36 Changed 9 years ago by Patrick Ruzand

gfx AMD tests OK for IE6, IE7, IE8 ; with VML or Silverlight renderer. Only one small failure, common to all browsers (FF, IE, Chrome, etc) : the test_refproj.html does not display the horizontal slider (but no errors displayed).

comment:37 in reply to:  36 ; Changed 9 years ago by Eugene Lazutkin

Replying to pruzand:

Only one small failure, common to all browsers (FF, IE, Chrome, etc) : the test_refproj.html does not display the horizontal slider (but no errors displayed).

That must be a dijit error. Please consult the current dijit slider tests to see what has changed.

comment:38 Changed 9 years ago by Eugene Lazutkin

There is one problem that bites gfx --- it may be worth spending some time on that. VML has bugs in standards mode, so we are forced to use quirks. It means that we will be in quirks on all other browsers too. Now quirks has a lot of CSS problems in different browsers, and IE (all versions) behaves differently too.

Can somebody convert our tests to standards mode and see what is broken and why? Bonus point for fixing found problems with VML. ;-)

comment:39 in reply to:  37 ; Changed 9 years ago by cjolif

Replying to elazutkin:

Replying to pruzand:

Only one small failure, common to all browsers (FF, IE, Chrome, etc) : the test_refproj.html does not display the horizontal slider (but no errors displayed).

That must be a dijit error. Please consult the current dijit slider tests to see what has changed.

The parser is now not anymore automatically imported by dijit, you have to explicitly require it in the test. I had to do that to several charting tests to make them work. I suspect this is the same issue here.

comment:40 Changed 9 years ago by Chris Mitchell

Will open new ticket to clean up gfx tests... Need STD mod, quirks mode, sync & async to be complete... Also HTML vs html5 have some effect on gradients at least on safari

comment:41 Changed 9 years ago by Chris Mitchell

Maybe php could be our friend here

comment:42 in reply to:  39 Changed 9 years ago by Patrick Ruzand

Replying to cjolif:

Replying to pruzand:

Only one small failure, common to all browsers (FF, IE, Chrome, etc) : the test_refproj.html does not display the horizontal slider (but no errors displayed).

The parser is now not anymore automatically imported by dijit, you have to explicitly require it in the test. I had to do that to several charting tests to make them work. I suspect this is the same issue here.

You're right. dojo.require("dojo.parser") did the trick. Thanks!

comment:43 Changed 9 years ago by Chris Mitchell

(In [24857]) fixes #12844 charting/amd code cleanup from cjlif !strict

comment:44 Changed 8 years ago by bill

(In [25844]) Fix AMD conversion errors. Refs #12844 !strict.

comment:45 Changed 8 years ago by bill

Resolution: fixed
Status: closedreopened

Not quite completed yet:

  • gfx
    • fx.js is using dojo as a global
    • util.js accessing dojo.style but dojo is not defined and dom-style.js is not required. same for some other dojo references in that file
    • vml.js accessing dojo as a global
  • charting
    • Chart3d.js accessing dojo.byId
    • DataChart.js accessing dojo.isArray, dojo.map
    • base.js accessing dojo.declare, dojo.isString
    • ChartAction.js accessing dojo.declare
    • Highlight.js accessing dojo.declare, dojo.connect
    • Magnify.js accessing dojo.declare, dojo.connect
    • MouseIndicator.js accessing dojo.declare, dojox.charting.action2d.ChartAction?, dojo.isIE, dojo.forEach, etc.

There are more broken files too, I don't want to list them all.

comment:46 Changed 8 years ago by Chris Mitchell

Resolution: fixed
Status: reopenedclosed

In [26015]:

fixes #12844 misc gfx and charting amd reference cleanups. thanks for pointing these out bill. \!strict

comment:47 Changed 8 years ago by Chris Mitchell

In [26027]:

refs #12844 more global reference fixes for charting \!strict

comment:48 Changed 8 years ago by bill

Resolution: fixed
Status: closedreopened

Thanks, it's looking better. I still see a few references to dojo.isArray, dojo.map, dojo.number, dojo.declare, etc. in charting.

comment:49 Changed 8 years ago by Chris Mitchell

Resolution: fixed
Status: reopenedclosed

In [26043]:

fixes #12844 few remaining AMD references \!strict

comment:50 Changed 8 years ago by bill

Resolution: fixed
Status: closedreopened

Thanks.

In charting, you still have some dojo references in BidiSupport.js, plus dijit references in a few places. (Should use Tooltip.show() / Tooltip.hide(), not dijit.showTooltip() and dijit.hideTooltip())

In gfx, still a reference to dojo.body().

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

comment:51 Changed 8 years ago by Chris Mitchell

Resolution: fixed
Status: reopenedclosed

In [26055]:

fixes #12844 more refs to dijit and dojo in charting, gfx. thx for finding these bill. \!strict

comment:52 Changed 8 years ago by Chris Mitchell

In [26056]:

refs #12844 AMD bug fixes in charting.. \!strict

comment:53 Changed 8 years ago by bill

Resolution: fixed
Status: closedreopened

OK, remaining stuff I see is:

  • dijit._Widget
  • strange kernel.scopeMap.dojo._scopeName, kernel.scopeMap.dojox._scopeName, looks like a bug?
  • unneeded references to dojox, for example:
declare("dojox.charting.DataChart", dojox.charting.Chart

The "dojox.charting.DataChart" is OK, but the dojox.charting.Chart reference should just be Chart, where "./Chart" is in the require list.

Some other examples are:

declare("dojox.charting.Series", dojox.charting.Element

and

var def = dojox.charting.Theme.defaultTheme;

and

var theme = new dojox.charting.Theme

I didn't list them all because looks like the dojox references haven't been worked on yet.

comment:54 Changed 8 years ago by Chris Mitchell

originally, i had converted all the dojox.XXX references in declare statements as you suggest, but i backed them out because it broke api docs for chains. Pete had a few suggestions for how to work around this, but could get none to actually work. I don't plan to fix these until the doc parser works properly or we know how to doc them (this is the second or third time this issue has come up and i still dont see a solution).

Need uhop to take a look at the sopemap conversion. Gfx was storing element ids in the dojo and dojox scopemaps, I think this is working as it was before, but need him to take a look.

will try to find the dijit._Widget ref

comment:55 Changed 8 years ago by bill

Hmm, if that's true then we have a serious issue, since all of the dojo codebase has been converted in the same way. Can you give an example of a file that you couldn't convert, and what exactly appeared incorrectly in preview.php?

comment:56 Changed 8 years ago by Chris Mitchell

apparently pete fixed the prob in preview.php, as it's able to parse properly when I use the var pseudos now. Will be checking in updates for gfx and charting soon.

comment:57 Changed 8 years ago by Chris Mitchell

In [26195]:

refs #12844 update remaining dojox refs in declare statements in gfx \!strict

comment:58 Changed 8 years ago by bill

As to the remaining dojox references in gfx, outside of declare statements, I'll attach an untested patch for removing them, please take a look.

comment:59 Changed 8 years ago by Chris Mitchell

In [26226]:

refs #12844 remove dojox from declares in charting modules \!strict

comment:60 Changed 8 years ago by Chris Mitchell

In [26227]:

refs #12844 remove dojox from declares in charting modules \!strict

comment:61 Changed 8 years ago by Chris Mitchell

In [26228]:

refs #12844 remove dojox from declares in charting modules \!strict

comment:62 Changed 8 years ago by Chris Mitchell

In [26229]:

refs #12844 fix api doc parsing issues in gfx modules \!strict

comment:63 Changed 8 years ago by Chris Mitchell

In [26246]:

refs #12844 more AMD updates for gfx from wildbill. Thanks! \!strict

comment:64 Changed 8 years ago by Chris Mitchell

In [26247]:

refs #12844 add async gfx testcase \!strict

comment:65 Changed 8 years ago by Chris Mitchell

In [26249]:

refs #12844 AMD updates for gfx3d \!strict

comment:66 Changed 8 years ago by Chris Mitchell

In [26250]:

refs #12844 more AMD updates for charting \!strict

comment:67 Changed 8 years ago by Chris Mitchell

In [26251]:

refs #12844 updates for dojox.gfx3d for minimal AMD dependencies

comment:68 Changed 8 years ago by liucougar

In [26252]:

refs #12844: fix an AMD conversion problem

matrix module returns an object and Matrix2D is the constructor for matrix now

comment:69 Changed 8 years ago by bill

In [26254]:

fix typo from [29246], refs #12844 !strict

comment:70 Changed 8 years ago by Chris Mitchell

In [26264]:

refs #12844 bug in scheduler return value.

comment:71 Changed 8 years ago by Chris Mitchell

In [26271]:

refs #12844 patch for amd port bug in gfx/utils. thx pruzand IBM CCLA

comment:72 Changed 8 years ago by Chris Mitchell

In [26277]:

refs #12844 dynamically load tooltip and number modules when they've been explicitly required by dev. was broken in amd port \!strict

comment:73 Changed 8 years ago by Chris Mitchell

need to take a pass through api and refguide docs, and mdurocher is finalizing patch for geo/openlayers.

comment:74 Changed 8 years ago by Chris Mitchell

In [26279]:

refs #12844 api doc updates for gfx and charting thanks pruzand IBM and cjolif IBM \!strict

comment:75 Changed 8 years ago by Chris Mitchell

In [26280]:

refs #12844 api doc updates for charting actions \!strict

comment:76 Changed 8 years ago by Chris Mitchell

In [26281]:

refs #12844 fix for AMD port in gauges where tooltip and number were only used if explicitly required by dev \!strict

comment:77 Changed 8 years ago by Chris Mitchell

In [26288]:

refs #12844 update css3 to follow style guidelines, thx snover \!strict

comment:78 Changed 8 years ago by Chris Mitchell

In [26304]:

refs #12844 AMD updates for dojox.gauges. Thx Emmanuel IBM CCLA \!strict

comment:79 Changed 8 years ago by Chris Mitchell

In [26305]:

refs #12844 AMD updates for mobileGauges demo. Thx Emmanuel IBM CCLA \!strict

comment:80 Changed 8 years ago by Chris Mitchell

In [26306]:

refs #12844 AMD updates for mobileGallery demo. thx Archer IBM CCLA \!strict

comment:81 Changed 8 years ago by Marc Durocher

Added OpenLayers? amd patch

comment:82 Changed 8 years ago by Chris Mitchell

In [26312]:

refs #12844 updates to geo.openlayers for AMD with min deps, and fixed testcases to follow naming conventions. thx marc durocher@ibm \!strict

comment:83 Changed 8 years ago by Chris Mitchell

In [26313]:

refs #12844 updates to geo.openlayers for AMD with min deps, and fixed testcases to follow naming conventions. thx marc durocher@ibm \!strict

comment:84 Changed 8 years ago by Chris Mitchell

In [26314]:

refs #12844 amd updates and cleanup for geo.charting. \!strict

comment:85 Changed 8 years ago by Chris Mitchell

In [26325]:

refs #12844 more amd updates for charting themes and tests
\!strict

comment:86 Changed 8 years ago by Chris Mitchell

In [26327]:

refs#12844 'on' handling bug from AMD updates to mobile gallery \!strict

comment:87 Changed 8 years ago by bill

Actually from searching the codebase for

on\(.*"on

looks like there are some more mistakes like above.

Plus which IIUC on() is for DOMNodes, for attaching to plain javascript methods should use aspect.after().

comment:88 Changed 8 years ago by Chris Mitchell

In [26328]:

refs #12844 AMD updates to charting theme tests for plotkit issue. \!strict

comment:89 Changed 8 years ago by Chris Mitchell

In [26337]:

refs #12844 cleanup formating in amd test cases for charting \!strict

comment:90 Changed 8 years ago by Chris Mitchell

In [26342]:

refs #12844 cleanup formating in amd test cases for charting \!strict

comment:91 Changed 8 years ago by cjolif

I see this is still open, what are we missing here?

comment:92 Changed 8 years ago by bill

I don't know any remaining issues.

comment:93 Changed 8 years ago by cjolif

Resolution: fixed
Status: reopenedclosed

Thanks Bill. So I'm going to close this one. If someone found additional issues later on, please re-open.

comment:94 Changed 7 years ago by cjolif

In [28722]:

refs #12844. AMD fix: correctly get a hand on dojox.

Note: See TracTickets for help on using tickets.