Opened 9 years ago

Last modified 8 years ago

#11869 closed enhancement

Refactor modules to use CommonJS AMD API — at Version 45

Reported by: Kris Zyp Owned by: Rawld Gill
Priority: high Milestone: 1.6
Component: Loader Version: 1.5
Keywords: Cc: Adam Peller
Blocked By: Blocking:

Description (last modified by Rawld Gill)

Refactor all the Dojo and Dijit modules to use the CommonJS AMD API. This module API can be used with top level scripts, allowing us to easily use an asynchronous loader that simply insert script elements.

The scope of this ticket is limited to converting to AMD module format and operability with v1.x sync and xdomain loaders and build system. Use #11893 for work required to get Dojo and Dijit fully operational with an AMD-compliant, async loader.

Change History (46)

comment:1 Changed 9 years ago by Kris Zyp

Milestone: tbd1.6
Type: defectenhancement

comment:3 Changed 9 years ago by Kris Zyp

(In [23032]) Refactor modules to CommonJS AMD, !strict refs #11869

comment:4 Changed 9 years ago by Kris Zyp

(In [23033]) Refactor modules to CommonJS AMD, !strict refs #11869

comment:5 Changed 9 years ago by Kris Zyp

(In [23034]) Fixed syntax error in query-sizzle for refactoring modules to CommonJS AMD, !strict refs #11869

comment:6 Changed 9 years ago by Kris Zyp

(In [23035]) Refactor modules to CommonJS AMD, !strict refs #11869

comment:7 Changed 9 years ago by Kris Zyp

(In [23036]) Refactor modules to CommonJS AMD, !strict refs #11869

comment:8 Changed 9 years ago by haysmark

In IE8 in test_TitlePane, I get an error, "Could not load class 'dijit.TestTitlePane?". Please investigate. I do not get this in the previous nightly.

comment:9 Changed 9 years ago by Kris Zyp

Owner: changed from Kris Zyp to Rawld Gill

comment:10 Changed 9 years ago by ben hockey

perhaps this patch (following) fixes it.

Changed 9 years ago by ben hockey

Attachment: 11869-TitlePane.diff added

comment:11 Changed 9 years ago by Kris Zyp

(In [23037]) Applies neonstalwart's patch for test_TitlePane.html, refs #11869

comment:12 Changed 9 years ago by Eugene Lazutkin

Description: modified (diff)

comment:13 Changed 9 years ago by haysmark

dijit/tests/test_Tree.html regressed as well.

comment:14 Changed 9 years ago by ben hockey

also, it looks like dijit._Widget is missing 'dojo/Stateful' as a dependency

comment:15 Changed 9 years ago by Rawld Gill

(In [23041]) Reverted dijit._editor.RichText? to pre-AMD refactor behavior without using document.write, !strict refs #11869

comment:16 Changed 9 years ago by Rawld Gill

Status: newassigned

comment:17 Changed 9 years ago by Rawld Gill

(In [23043]) Fixed syntax error in test_TooltipDialog consequent to AMD refactor, !strict refs #11869

comment:18 Changed 9 years ago by Rawld Gill

(In [23044]) File improperly committed during initial AMD refactor [23032], !strict refs #11869

comment:19 Changed 9 years ago by Rawld Gill

(In [23045]) Fixes merge errors with dojo-sie during AMD refactor [23032], !strict refs #11869

comment:20 Changed 9 years ago by Rawld Gill

All dijit unit tests appear working as of [23045] (FF 3.6 on XP) except:

  • dijit.tests.robot.Tooltip_placement
  • dijit.tests.layout.ContentPane

These appear to be working about like prior to AMD refactor. Build rechecked and looks OK.

comment:21 Changed 9 years ago by ben hockey

(In [23046]) added dojo/Stateful dependency to dijit/_Widget, !strict refs #11869

comment:22 Changed 9 years ago by Kris Zyp

(In [23048]) Removing asynchronous loader files that should not be included yet, refs #11869

comment:23 Changed 9 years ago by Kris Zyp

(In [23049]) Removing asynchronous loader files that should not be included yet, refs #11869

comment:24 Changed 9 years ago by haysmark

See also #11869.

comment:25 Changed 9 years ago by haysmark

Err I mean #11883.

comment:26 Changed 9 years ago by Kris Zyp

(In [23053]) Make sync loader be fully AMD compliant and added tests, refs #11869 !strict

comment:27 Changed 9 years ago by Rawld Gill

(In [23059]) Added missing AMD inverse transform in build utility, fixes #11880 and #11888, refs #11869

comment:28 Changed 9 years ago by Rawld Gill

As of [23059] the following tests are failing in dijit.tests.module

dijit.tests.robot.Tooltip_placement
dijit.tests.layout.ContentPane
dijit.tests.editor.robot.Editor_mouse
dijit.tests.editor.robot.Editor_a11y
dijit.tests.editor.robot.BackForwardState
dijit.tests.form.Form

In each case, the resources appear to be loading correctly (both build and source). Module owners who think any of the above failures are consequent to the AMD refactor should open a separate ticket.

comment:29 Changed 9 years ago by Kris Zyp

(In [23060]) More robust regex for define() transformation, refs #11869

comment:30 Changed 9 years ago by Kris Zyp

(In [23061]) Properly coexist with preexisting define(), compatible with RequireJS now, refs #11869

comment:31 Changed 9 years ago by Kris Zyp

(In [23062]) Remove unnecessary dojo.require call, return proper dijit.Dialog, refs #11869 !strict

comment:32 Changed 9 years ago by Kris Zyp

Why are we converting all the AMD modules back to legacy format in the build? Can't we leave them in AMD format?

Also, why isn't dojo/_base.js and dojo/_base/browser.js in AMD format?

comment:33 Changed 9 years ago by haysmark

dojo/i18n.js, dojo/_base/query.js, and dojo/_base/query-sizzle.js also missed the refactor, but does it matter which format the files are in? Do we still support the old format?

comment:34 Changed 9 years ago by Kris Zyp

The bootstrap loading of base (with src) doesn't work with RequireJS because these modules are in the old format.

comment:35 Changed 9 years ago by ben hockey

i'm eager to see these (bootstrapping base and dojo/18n.js) addressed. currently, dojo.i18n.getLocalization looks directly to dojo._loadedModules for bundles. in my own code, i can avoid calling dojo.i18n.getLocalization, however, dojo.date.locale._getGregorianBundle uses it and i can't avoid that.

comment:36 in reply to:  32 ; Changed 9 years ago by Rawld Gill

Replying to kzyp:

The goal of the AMD refactor was to get interesting modules usable by new async systems without breaking 1.x with minimum changes and effort; not make the 1.x loaders and build util look/work/use AMD specs.

Why are we converting all the AMD modules back to legacy format in the build? Can't we leave them in AMD format?

Not converting modules back to 1.x format (as a sort of build preprocess filter) would require consideration/modification to the xdomain loader and multi-version support as well as further modifications to the build system. This seemed a questionable use of resources for systems that are nearing their end of (development) life.

Also, why isn't dojo/_base.js and dojo/_base/browser.js in AMD format?

I think it likely that these files will be subsumed by main.js in a properly constructed async system. (Something similar to this was done in dojo-sie). Therefore this likely not useful going forward.

comment:37 in reply to:  33 Changed 9 years ago by Rawld Gill

Replying to haysmark:

dojo/i18n.js, dojo/_base/query.js, and dojo/_base/query-sizzle.js also missed the refactor, but does it matter which format the files are in? Do we still support the old format?

They have been refactored. They look different because they must also must be stand-alone. Look towards the bottom of the files and you'll see the define calls.

comment:38 in reply to:  36 ; Changed 9 years ago by Kris Zyp

Replying to rcgill:

Replying to kzyp:

The goal of the AMD refactor was to get interesting modules usable by new async systems without breaking 1.x with minimum changes and effort; not make the 1.x loaders and build util look/work/use AMD specs.

dojo._base isn't interesting? Will it break anything if I check in an updated dojo/_base.js and dojo/_base/browser.js in AMD format? That seems to be all it takes to get src version running on RequireJS at this point.

comment:39 in reply to:  38 Changed 9 years ago by Rawld Gill

Replying to kzyp:

dojo._base isn't interesting? Will it break anything if I check in an updated dojo/_base.js and dojo/_base/browser.js in AMD format? That seems to be all it takes to get src version running on RequireJS at this point.

It shouldn't break anything. No objections to trying it. However, You'll need to load some of the dojo bootstrap to get dojo to work with requirejs. For example, set/getObject, are in _base/_loader/bootstrap.js. The boot was redone in dojo-sie if you want to look at that. I will have dojo-sie back up with the anon loader in the next day or so.

comment:40 in reply to:  35 Changed 9 years ago by Rawld Gill

Replying to neonstalwart:

i'm eager to see these (bootstrapping base and dojo/18n.js) addressed. currently, dojo.i18n.getLocalization looks directly to dojo._loadedModules for bundles. in my own code, i can avoid calling dojo.i18n.getLocalization, however, dojo.date.locale._getGregorianBundle uses it and i can't avoid that.

There is no current plan to do this as part of this refactor (maybe that opinion will change based on community goals). All the v1.x loader stuff is replaced (including dojo.i18n.getLocalization and dojo._loadedModules) with the new async loaders (requirejs and backdraft). Plan on guidance on these by 27 OCT.

comment:41 Changed 9 years ago by Kris Zyp

(In [23063]) Added dojo.isBrowser to the scope, to get conditional to pass through, refs #11869

comment:42 Changed 9 years ago by Kris Zyp

(In [23064]) _base modules converted to AMD, refs #11869

comment:43 Changed 9 years ago by Rawld Gill

dijit._Widget is not completely converted. Need to address pragma-bracketed dojo.require.

comment:44 Changed 9 years ago by ben hockey

requireJS needs the i18n bundle to use true to indicate available locales. a truthy value is not going to work - see http://github.com/jrburke/requirejs/blob/master/require/i18n.js#L94

should we update our i18n bundles to use true rather than 1?

comment:45 Changed 9 years ago by Rawld Gill

Description: modified (diff)
Note: See TracTickets for help on using tickets.