Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#11869 closed enhancement (fixed)

Refactor modules to use CommonJS AMD API

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.

Attachments (5)

11869-TitlePane.diff (740 bytes) - added by ben hockey 9 years ago.
i18n.diff (1.4 KB) - added by Kris Zyp 9 years ago.
11869-i18n.diff (1.7 KB) - added by ben hockey 9 years ago.
bootstrap-as-amd.diff (3.7 KB) - added by Kris Zyp 9 years ago.
Make bootstrap files properly load through AMD so Dojo can be used as a package a module loader
package-main.js (350 bytes) - added by Kris Zyp 9 years ago.
C:\dev\dojo-toolkit\dojo\_base\_loader\package-main.js

Download all attachments as: .zip

Change History (119)

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)

comment:46 Changed 9 years ago by Rawld Gill

(In [23068]) Revert savetextarea changes in AMD refactor in RichEdit? (they did not work); !strict, refs #11869 and #11893

comment:47 Changed 9 years ago by Kris Zyp

(In [23073]) Declare define as a top-level variable to fix IE stack overflow issue, refs #11869

comment:48 in reply to:  28 Changed 9 years ago by bill

Replying to rcgill:

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.

  • dijit.tests.robot.Tooltip_placement - fixed in [23065]
  • dijit.tests.layout.ContentPane
    - open as #11840
  • dijit.tests.editor.robot.Editor_mouse - repopened #11300
  • dijit.tests.editor.robot.Editor_a11y - repopened #11300
  • dijit.tests.editor.robot.BackForwardState - you fixed this with [23068]
  • dijit.tests.form.Form - this is working for me

comment:49 Changed 9 years ago by ben hockey

(In [23074]) changing i18n bundles to be compatible with requireJS. strict, refs #11869

comment:50 Changed 9 years ago by ben hockey

(In [23075]) changing i18n bundles to be compatible with requireJS. strict, refs #11869

comment:51 in reply to:  49 ; Changed 9 years ago by Rawld Gill

Replying to neonstalwart:

(In [23074]) changing i18n bundles to be compatible with requireJS. strict, refs #11869

I don't understand the need for this (or [23075]). Truthy is good enough and saves bytes.

comment:52 in reply to:  51 Changed 9 years ago by Kris Zyp

Replying to rcgill:

Replying to neonstalwart:

(In [23074]) changing i18n bundles to be compatible with requireJS. strict, refs #11869

I don't understand the need for this (or [23075]). Truthy is good enough and saves bytes.

http://github.com/jrburke/requirejs/blob/master/require/i18n.js#L94

comment:53 in reply to:  51 Changed 9 years ago by ben hockey

Replying to rcgill:

Replying to neonstalwart:

(In [23074]) changing i18n bundles to be compatible with requireJS. strict, refs #11869

I don't understand the need for this (or [23075]). Truthy is good enough and saves bytes.

simply compatability. i understand saving bytes - if requireJS changes then we can move back. so far, i haven't made any request for it to be changed, feel free to do so if you think it's needed.

comment:54 Changed 9 years ago by James Burke

For context, RequireJS uses true check explicitly because it is allowed for the value to be an object with the actual string translations. Testing explicitly for true value was the way I went for it. But open to other ideas. I could explicitly test for 1 in addition to true.

comment:55 Changed 9 years ago by James Burke

Although, I suppose it is nice to have one way to say it. With gzipping the size cost should not be that different? Open to other arguments though.

comment:56 Changed 9 years ago by ben hockey

i don't have a strong preference either way - my immediate goal was compatibility.

comment:57 in reply to:  54 ; Changed 9 years ago by Rawld Gill

Replying to jburke:

For context, RequireJS uses true check explicitly because it is allowed for the value to be an object with the actual string translations. Testing explicitly for true value was the way I went for it. But open to other ideas. I could explicitly test for 1 in addition to true.

OK, that makes more sense. And I like the idea of being able to include a localized bundle in a root bundle very much.

In general, I prefer testing for truthy whenever possible as it tends to be less brittle.

On this particular issue, I would prefer to see (1) a test for isObject indicates a bundle; otherwise, (2) truthy indicates a bundle is available in a localized resource; otherwise, (3) no localized bundle is available (for a given locale).

I also observe that the AMD spec seems pretty well vetted and formalized, while the various plugins with requirejs are less so. Who ultimately owns the i18n! and text! specs? commonJS? dojo? requireJS? Whatever the answer, we need to formalize these important plugins a bit more.

comment:58 Changed 9 years ago by Rawld Gill

@neonstalwart: no problem with change

I think we should also remove the explicit module ids in the i18n bundles, e.g.

define("i18n!dojo/nls/colors",

These are an artifact of requirements for older versions of various async loaders.

I'll do that unless someone beats me to it.

comment:59 in reply to:  57 ; Changed 9 years ago by Kris Zyp

Replying to rcgill:

Replying to jburke:

[snip]

I also observe that the AMD spec seems pretty well vetted and formalized,

Some on the CommonJS ML might suggest otherwise :). But I am happy with it.

while the various plugins with requirejs are less so. Who ultimately owns the i18n! and text! specs? commonJS? dojo? requireJS? Whatever the answer, we need to formalize these important plugins a bit more.

Yes, right now the plugins are just in the RequireJS API. I have been hesitant to engage in another proposal with CommonJS considering how out of hand AMD has become. On the other hand, maybe a plugin proposal would be good distraction from the endless discussion on injection variables on the CommonJS ML :). Anyway, I was actually kind of waiting on James Burke to take a look at a minimal possible spec for plugins. I had proposed something very simple, and I think he needs to figure out how feasible that is.

comment:60 in reply to:  59 Changed 9 years ago by James Burke

Replying to kzyp:

Yes, right now the plugins are just in the RequireJS API. I have been hesitant to engage in another proposal with CommonJS considering how out of hand AMD has become. On the other hand, maybe a plugin proposal would be good distraction from the endless discussion on injection variables on the CommonJS ML :). Anyway, I was actually kind of waiting on James Burke to take a look at a minimal possible spec for plugins. I had proposed something very simple, and I think he needs to figure out how feasible that is.

I would rather not bring up plugins on the CommonJS group at this time. I am exhausted from the recent discussion, I am tired of feeling like bringing up topics that are not interesting to most of the participants, and I want to get some actual coding done for a bit.

What Kris Zyp proposed earlier for a plugin API should be the ideal target. I think we need to also have a define: function(){} api as well as load: in a plugin spec. The define would be for i18n modules that are loaded/they need to be integrated into the complete bundle.

My current focus though has been a refactor to make it possible to execute modules as soon as their dependencies are ready (vs waiting until all outstanding scripts are fetched). I felt this work is needed to get a has! plugin to work properly where it will likely need to fetch some has tests on demand before resolving a has! dependency. That, and reusing define args across contexts, something that should help Skywriter.

So my focus on the moment is not plugin architecture. However, if you want to start the discussion feel free to do so. I may not jump in right away. And I'm a bit gun-shy of the CommonJS group at the moment.

As to Rawld's suggestion of an isObject test, I normally do not prefer them because it can be a trickier check if you want an object that is not a function, for example. The direct true one seemed cheaper and less code, but I could be wrong on that.

comment:61 in reply to:  59 ; Changed 9 years ago by Rawld Gill

Replying to kzyp:

[snip]

I had proposed something very simple, and I think he needs to figure out how feasible that is.

Can you point me to this proposal.

comment:62 Changed 9 years ago by Rawld Gill

For dojo, v1.6, it seems the critical thing to get right is the module format(s). As I understand the decisions to date:

  1. AMD, for modules.
  2. i18n bundles are just AMD modules.
  3. text (e.g., dojo's template files) are just text files with no rules at all.

We can figure out various optimized ways to consume these resources going forward.

comment:63 in reply to:  61 Changed 9 years ago by James Burke

Replying to rcgill:

Can you point me to this proposal.

I think this was it: http://groups.google.com/group/commonjs/msg/45c226a9cfb6ebd1

comment:64 Changed 9 years ago by Kris Zyp

Here is my (little more thought through) proposed plugin system and what I think we should update in our Dojo modules. I could put the plugin system proposal in CommonJS at some point, but the important implementors are all on this ticket anyway.

Proposed plugin system A plugin is a module that is used to resolve module ids of the form "plugin!id". A plugin is a module that exports a load function. To resolve a module id, a module loader shall first use the string before the "!" character as the id to resolve the plugin module using standard module lookup. The module loader shall then call the load(args) function on the plugin module to resolve the full id. The load function shall be called with a single object as an argument. The object shall have these properties:

  • id - The string after the "!" character.
  • loaded - A function that the plugin shall call when the id has been resolved. The function shall be called with a single argument, the value that the id resolves to.
  • require - This is the require function that was or would have been (if "require" had indicated as a injection variable) provided to the calling module, that is the module requesting or depending on the module currenlty being resolved. This require should handle requested relative module ids using the calling module as the context for the relative resolution.

The require function would also have the following functions as properties require.ensure or require.async - We need a function to do an asynchronous require (with a callback). We could use CommonJS's require.ensure() as a property of the "require" free variable (the injection variable, in our case) or we could use Node's require.async() (also a property of the "require" free variable). I prefer a property of the free variable since having an injected "require" makes it difficult to use the global at the same time.

require.url(name) - This would take a resource name (including relative names) and return a URL (that could be used for XHR requests, so it doesn't necessarily need to be an absolute URL). The name should include the file extension (if it has one).

require.when - Event registration for notification of module requests would be important for implementing various other plugins. This is actually not needed for the core plugins needed by Dojo (text and i18n). This would take the form require.when(moduleId, callback) and would be called as soon as a module is requested for asynchronous loading. There is no event for when a module becomes available for synchronous download, but require.when plus require.async can be used to simulate such an event.

Alternate plugin approach. We could use RequireJS's require() using the global "require" function as an asynchronous loader, have a resolveURL and resolveId on the load args object to perform relative module id resolution, and the when() could be on the global require or define variables. I prefer the first approach because it keeps a cleaner API, and it doesn't rely on an additions to the global variables ("require" or "define"). However, this alternate may be more appealing to James, I am not sure.

Suggested Dojo changes:

  • We add support for plugins using this API.
  • We will create dojo/i18n and dojo/text plugin.
  • dojo.cache() calls should be replaced with an injected variable using "dojo/text" plugin like "dojo/text!./resources/Tree.html" (this would be the template used by dijit/Tree, for example).
  • NLS bundles should be packaged as anonymous packages.
  • NLS bundles should be referenced using "dojo/i18n" as the plugin name, like "dojo/i18n!dojo/nls/colors".

The dojo/text plugin could look like:

define("dojo/text", dojo?, function(dojo){

return {

load: function(args){

dojo.xhrGet({

url: args.require.url(args.id);

}).then(function(response){

args.loaded(response);

});

}

});

comment:65 Changed 9 years ago by Katie Vance

I think the new syntax define(....) is causing javascript errors on opera 10.63. I'm seeing this issue every time I load a test file on opera:

failed loading ../../../dojo/./_firebug/firebug.js with error: ReferenceError?: Undefined variable: define

Please let me know if this is related.

comment:66 Changed 9 years ago by Katie Vance

Cc: Katie Vance added

comment:67 in reply to:  6 Changed 9 years ago by ben hockey

Replying to kzyp:

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

after building dojo, define is no longer around and we need to use dojo.provide whenever dojo is defined. in doh/runner.js, would it work to change

if (typeof dojo !== "undefined") {
  define("doh/runner", [], function(){d(doh);});
} else {
  d(doh);
}

to something like this:

if (typeof define !== "undefined") {
  define("doh/runner", [], function(){d(doh);});
} else {
  if (typeof dojo !== "undefined") {
  	dojo.provide("doh.runner");
  }
  d(doh);
}

so that the tests can be run with code that's been built? whatever the solution is, the same change would be needed in _browserRunner.js as well.

comment:68 Changed 9 years ago by ben hockey

(In [23104]) getting the AMD changes to the doh runner working with a built version of dojo, !strict, refs #11869

comment:69 Changed 9 years ago by Kris Zyp

(In [23117]) Fix Opera define scope bug, fix anonymous module/i18n bundle support, make all i18n bundles be anonymous, refs #11869 !strict

comment:70 Changed 9 years ago by Kris Zyp

(In [23118]) make all i18n bundles be anonymous, refs #11869 !strict

comment:71 Changed 9 years ago by Rawld Gill

(In [23125]) fixed module tests to load properly (though still fail) with built code, refs #11869

comment:72 Changed 9 years ago by Kris Zyp

It seems like we at least still need to add text! plugin dependencies to all the dijit modules that use dojo.cache so that templates can be asynchronously loaded. Arguably we should be using the injected value from the text! dependency, but keeping the dojo.cache call to get the text after it has been loaded/cached is fine. Any objections to getting this added? For the sync setup it would just be a no-op for now (unless we use the injected value).

comment:73 in reply to:  72 Changed 9 years ago by ben hockey

Replying to kzyp:

It seems like we at least still need to add text! plugin dependencies to all the dijit modules that use dojo.cache so that templates can be asynchronously loaded. Arguably we should be using the injected value from the text! dependency, but keeping the dojo.cache call to get the text after it has been loaded/cached is fine. Any objections to getting this added? For the sync setup it would just be a no-op for now (unless we use the injected value).

i agree that dojo.cache usage should be converted to a text! plugin dependency.

after doing some more work trying to convert and build some dojox modules, i can make a general comment about using injected values - with the way the build tool works, you should only use the injected value of anything that would have otherwise been a global and you should only inject it using the same name as the global. to demonstrate:

define(["dojo", "text!dojo/template/foo.html"], function (d, foo) {    // this line goes away when built - d and foo are not defined
    function moduleDef() { return foo };    // foo is undefined when built

    d.foo = moduleDef;    // d is undefined when built

    return d.foo;    // this line goes away when built
});    // this line goes away when built

as you can see, it's necessary to use dojo.cache if you are going to use dojo's build tool.

comment:74 Changed 9 years ago by Adam Peller

Cc: Adam Peller added

(In [23138]) change dojo.cldr generation scripts to use AMD format

comment:75 Changed 9 years ago by Adam Peller

(In [23139]) Regenerate dojo.cldr based on scripts for new AMD format and CLDR 1.9rc data. Refs #11884, #11869

comment:76 Changed 9 years ago by ben hockey

(In [23142]) * fixing typo in module definition

  • making sure that dojo.i18n is defined
  • returning dojo.i18n

refs #11869 !strict

comment:77 Changed 9 years ago by Kris Zyp

(In [23143]) Added template dependencies using text! plugin to all Dijit modules that use dojo.cache, refs #11869 !strict

comment:78 Changed 9 years ago by ben hockey

(In [23175]) returning doh from module definition. refs #11869 !strict

comment:79 Changed 9 years ago by bill

(In [23184]) Cleanup from dojo SIE conversion. open2dialogs() wasn't being defined when needed, and the other stuff in dojo.addOnLoad() was vestigial. Refs #11869.

comment:80 Changed 9 years ago by ben hockey

(In [23257]) the arguments passed on to originalDefine were wrong for i18n bundles because we changed the name argument before calling originalDefine. refs #11869 !strict

comment:81 Changed 9 years ago by ben hockey

i'm coming across a problem with trying to use dojo when define already exists. the code to hook into the original define calls is failing when trying to load dojo's i18n files.

the hook is trying to sniff for i18n files by seeing if the name passed to define begins with "i18n". however, our i18n modules are defined like so:

define({ root:

//begin v1.x content
{
    // the v1.x i18n stuff...
}
//end v1.x content
,
	"ar": true,
	"ca": true,
	"cs": true // ... etc
});

because we aren't using a name in our call to define, our hook into the existing define fails and i18n isn't working when using dojo from source with an existing define.

any thoughts on how to work around this?

comment:82 in reply to:  81 Changed 9 years ago by ben hockey

to clarify:

by "failing" i mean that the logic to detect i18n files is not catching these files as being i18n files. the code itself is not failing (as in throwing errors or whatever) - the logic to detect i18n files is not sufficient and thus "failing".

comment:83 in reply to:  81 ; Changed 9 years ago by Rawld Gill

Replying to neonstalwart:

Your use case is beyond the scope of what we are supporting with 1.6 release. The AMD simulation in the loader has already seen some serious scope creep, and I'm hesitant to add more capabilities. Certainly, there is nothing in the v1.5-6 build system that can currently handle anonymous modules.

That said, I'll bring this up at the meeting on Wednesday (scope creep etc.) and see if we're going to move the line again.

comment:84 in reply to:  83 Changed 9 years ago by ben hockey

Replying to rcgill:

Your use case is beyond the scope of what we are supporting with 1.6 release. The AMD simulation in the loader has already seen some serious scope creep, and I'm hesitant to add more capabilities. Certainly, there is nothing in the v1.5-6 build system that can currently handle anonymous modules.

That said, I'll bring this up at the meeting on Wednesday (scope creep etc.) and see if we're going to move the line again.

i'm hoping that there is a consensus to address this.

i don't think that fixing a feature that exists in the code is creeping the scope. the code in dojo/_base/_loader/loader.js attempts to support an existing define but it doesn't quite manage to do it successfully. i'd like to get existing code to do what it looks like it was attempting to do.

i've been working with requirejs to have changes implemented which will support loading and building dojo as it exists in trunk and this is what i hope is the final step in having a very simple way to load dojo with requirejs from source and after being built with the requirejs build tool.

at the moment, both code bases are very close to being able to support loading a source and built version of dojo simply by adding a dojo.js as a sibling of dojo, dijit, dojox folders. we are very close to where the contents of dojo.js would be:

require([
	'order!dojo/_base/_loader/bootstrap.js',
	'order!dojo/_base/_loader/loader.js',
	'order!dojo/_base/_loader/hostenv_browser.js',
	// this list of includes is needed because of the conditional used in dojo/_base.js
	'order!dojo/_base/lang',
	'order!dojo/_base/array',
	'order!dojo/_base/declare',
	'order!dojo/_base/connect',
	'order!dojo/_base/Deferred',
	'order!dojo/_base/json',
	'order!dojo/_base/Color',
	'order!dojo/_base/browser'
], function () {});

currently, i have this working with a build but i have to provide an additional shim to get it working from source (wrapped in require.ready):

function init() {
    require(["my/app"], function (app) {
        app.init();
    });
}

if (!usingBuild) {
	require([
		'dojo',
		'i18n!dojo/cldr/nls/gregorian'
	], function (d, gregorian) {
		d._loadedModules = d._loadedModules || {};
		d._loadedModules['dojo.cldr.nls.gregorian'] = { ROOT: gregorian };
		init();
	});
}
else {
	init();
}

the biggest reason i see this as a bug rather than an addition to the scope is because our code that wraps an existing define is looking for a name that begins with "i18n!" to sniff out i18n modules but none of our i18n modules are defined in a way that our own code is able to detect that they are i18n modules when they're loaded. if we are going to support wrapping an existing define then our code should at least be able to load itself that way.

i see this leaving 2 options

  • don't support wrapping an existing define
  • get our code to work (either change the way we wrap define or change the definition of our i18n modules)

i would be disappointed if we don't support wrapping an existing define but i feel like it would be more consistent than attempting to support it but failing to get it right.

admittedly, this is probably outside of the scope of what was intended for this particular ticket and #11893 was probably the ticket i should have used to address this.

comment:85 Changed 9 years ago by Kris Zyp

Maybe rather than trying to intercept i18n bundles on load, we should have i18n.js use a require() call (using the "require" injection variable from RequireJS) rather than getting the bundle from dojo._loadedModules[module] in line 38 of i18n.js.

Changed 9 years ago by Kris Zyp

Attachment: i18n.diff added

comment:86 Changed 9 years ago by Kris Zyp

Maybe something like the attached i18n.diff would work?

comment:87 Changed 9 years ago by Katie Vance

Cc: Katie Vance removed

comment:88 in reply to:  86 Changed 9 years ago by ben hockey

Replying to kzyp:

Maybe something like the attached i18n.diff would work?

i like that idea. it's close. of course the first line is right but i think the main change needs to be more like this.

	var module = [packageName,"nls",bundleName].join('.');
	//>>includeStart("asyncLoader", kwArgs.asynchLoader);
	var obj = require("i18n!" + [packageName.replace(/\./g, '/'), "nls", locale, bundleName].join('/'));
	(dojo._loadedModules[module] = dojo._loadedModules[module] || {})[elements.join('_')] = (obj.root || obj);
	//>>includeEnd("asyncLoader");
	var bundle = dojo._loadedModules[module];

and then we should be able to also remove the i18n code that wraps the originalDefine in loader.js and we will have reduced the size as well!

i'll report back after i've checked dojo source, dojo build, requirejs source and requirejs build... :)

Changed 9 years ago by ben hockey

Attachment: 11869-i18n.diff added

comment:89 Changed 9 years ago by ben hockey

the patch in 11869-i18n.diff should work.

comment:90 Changed 9 years ago by ben hockey

i committed r23279 but it didn't show up here

comment:91 Changed 9 years ago by John Hann

It seems that all of the dijits under dijit/form can't load their template. This is because the dependency points to the dijit/templates folder instead of dijit/form/templates. The reference to templateString: dojo.cache() has the same problem.

(I was hoping we'd replacing dojo.cache() with the text! plugin, but it seems we don't want to update the dojo build script to recognize the text! plugin?)

comment:92 in reply to:  91 Changed 9 years ago by John Hann

Replying to unscriptable:

It seems that all of the dijits under dijit/form can't load their template. This is because the dependency points to the dijit/templates folder instead of dijit/form/templates. The reference to templateString: dojo.cache() has the same problem.

Actually, I don't know what's wrong with dojo.cache. It's not being defined as a function in my tests, just a plain / empty object: {}.

comment:93 Changed 9 years ago by Kris Zyp

(In [23280]) Fix template references in dijit/form, refs #11869

comment:94 Changed 9 years ago by John Hann

I found the source of the dojo.cache problem. Line 1 of dojo/cache.js is:

define("dojo.cache", ["dojo"], function(dojo){

it should be:

define("dojo/cache", ["dojo"], function(dojo){

comment:95 in reply to:  93 Changed 9 years ago by John Hann

Replying to kzyp:

(In [23280]) Fix template references in dijit/form, refs #11869

Hey Kris, check out http://svn.dojotoolkit.org/src/dijit/trunk/form/CheckBox.js It's still looking in the wrong place. All of the others look good. Thx! :) -- John

comment:96 Changed 9 years ago by Kris Zyp

(In [23296]) Fix reference to template, refs #11869

comment:97 in reply to:  83 ; Changed 9 years ago by Kris Zyp

Replying to rcgill:

Replying to neonstalwart:

Your use case is beyond the scope of what we are supporting with 1.6 release. The AMD simulation in the loader has already seen some serious scope creep, and I'm hesitant to add more capabilities. Certainly, there is nothing in the v1.5-6 build system that can currently handle anonymous modules.

I don't mind removing some of the AMD functionality from the loader. Were you thinking of removing the "require", "exports", and "module" injection variables? However, I do want us to be able to follow good practices as we convert Dojox modules.

comment:98 Changed 9 years ago by ben hockey

(In [23353]) fixing some syntax errors in some dojox define calls !strict refs #11869

comment:99 Changed 9 years ago by ben hockey

(In [23375]) i18n is limited to the default locale when using require !strict refs #11869

comment:100 Changed 9 years ago by bill

(In [23403]) Remove some comments that are no longer attached to anything due to the AMD refactor, refs #11869.

comment:101 in reply to:  97 Changed 9 years ago by Rawld Gill

Replying to kzyp:

Replying to rcgill:

Replying to neonstalwart:

Your use case is beyond the scope of what we are supporting with 1.6 release.

[snip]

I don't mind removing some of the AMD functionality from the loader.

[snip]

Kris--thanks for all your help with the loader!

After letting this settle a bit, here's my opinion:

  1. Absolutely agree that best practices should be used in module conversions. This should be the key requirement for v1.6.
  1. We should leave all the AMD features we've added to the v1.5 sync loader for the v1.6 release. The purpose of these changes is to help early adopters as we/they convert various code stacks to AMD compliance.
  1. We should not talk too much about these. In particular, we should not advertise that the dojo sync loader is an AMD loader.
  1. At a minimum, we should provide an AMD-compliant, backwards-compatible loader/boot *and* build system for v1.7 (notice that it is fairly easy to make a good AMD loader handle the dojo require/provide api; therefore, backcompat should be easy.). This should be made available to 1.6 users--ideally ASAP after v1.6 release--either through dtk.org or unofficially other mean like dojo-sie.org.

4, Assuming success in [3], I tend to believe that the v1.6 sync and xdomain loader, boots, and build system should be deprecated in 1.7. The AMD reverse transforms in the v.16 builder are unabashedly brittle; the AMD simulation in the v1.6 sync loader may also have edge cases. I see no benefit to maintaining these scaffolds.

comment:102 Changed 9 years ago by ben hockey

(In [23409]) use the i18n plugin to load locale bundles. !strict refs #11869

comment:103 Changed 9 years ago by bill

(In [23461]) Convert test to run under AMD by getting rid of document.write() calls and moving dojo.declare() etc. inside of dojo.addOnLoad(), refs #3085, #11869.

comment:104 Changed 9 years ago by bill

(In [23462]) Convert test_ContentPane.html to load under AMD, to detect parse failures, and add to run as part of layout test suite, refs #11869.

comment:105 Changed 9 years ago by bill

(In [23463]) Move dojo.declare() calls inside dojo.addOnLoad(), for AMD compliance, refs #11869.

Changed 9 years ago by Kris Zyp

Attachment: bootstrap-as-amd.diff added

Make bootstrap files properly load through AMD so Dojo can be used as a package a module loader

Changed 9 years ago by Kris Zyp

Attachment: package-main.js added

C:\dev\dojo-toolkit\dojo\_base\_loader\package-main.js

comment:106 in reply to:  69 Changed 9 years ago by ben hockey

Replying to kzyp:

(In [23117]) Fix Opera define scope bug, fix anonymous module/i18n bundle support, make all i18n bundles be anonymous, refs #11869 !strict

http://bugs.dojotoolkit.org/browser/dojo/trunk/_base/_loader/loader.js#L834 dojo.provide makes a global var called anon when name is resolved as 'anon'.

can this be fixed with something like: var exports = dottedName === 'anon' ? {} : dojo.provide(dottedName);

comment:107 Changed 9 years ago by Sam Foster

(In [23592]) Adding AMD wrapper. Refs #11869

comment:108 Changed 9 years ago by Kenneth G. Franqueiro

[23633] refs this ticket. (Commit message eluded Trac's sensor)

comment:109 Changed 9 years ago by bill

(In [23998]) rollback strange spacing changes from [23032], refs #11869

comment:110 Changed 9 years ago by Rawld Gill

Resolution: fixed
Status: assignedclosed

See #11893 for further development of AMD dojo.

comment:111 Changed 9 years ago by bill

Component: GeneralLoader

comment:112 Changed 8 years ago by liucougar

(In [25802]) refs #11869: added doh profile and package.json for new builder

comment:113 Changed 8 years ago by bill

(In [25804]) Fix AMD conversion error (missing definition for doh variable), refs #11869 !strict. Thought about getting "doh" from "doh/robot" rather than "doh/_browserRunner" (or "doh/runner'), but currently the "doh/robot" module doesn't return anything.

comment:114 Changed 8 years ago by bill

In [26259]:

eugene's patch so safeMixin() available through AMD exports, and so it's monkey-patchable, !strict refs #11869

Note: See TracTickets for help on using tickets.