Opened 22 months ago

Closed 12 months ago

Last modified 12 months ago

#15616 closed defect (fixed)

make dojo work with other AMD loaders

Reported by: jburke Owned by: csnover
Priority: blocker Milestone: 1.9
Component: General Version: 1.8.0b1
Keywords: Cc: skaegi, dylan, jamesthomas, rbackhouse, rcgill
Blocked by: #15866 Blocking:

Description

Trying dojo 1.8b1, and dojo/ready calls require.on(), where ideally there is a check for require.on done before calling it.

This allows other loaders, like requirejs, which do not place an "on" on require to load the file. I did not do a search, but if there are other uses of require.on, they should also get wrapped in a check.

Change History (54)

comment:1 Changed 22 months ago by bill

  • Owner set to rcgill
  • Status changed from new to assigned

Same as #14587?

comment:2 Changed 22 months ago by rcgill

  • Resolution set to duplicate
  • Status changed from assigned to closed

This is a dup of #14587.

comment:3 Changed 22 months ago by peller

  • Cc skaegi added
  • Resolution duplicate deleted
  • Status changed from closed to reopened

Suggesting we reopen since #14587 is closed and there is an interop issue here. We could rework the dijit/* calls to avoid loading dojo/ready or tackle the require API issue in dojo/ready directly. @jburke, perhaps we need common API support to make this happen?

I'd prefer we have a solution before 2.0 so that everyone on the Dojo 1.x line can use alternate loaders, including requirejs and zazl

comment:4 Changed 22 months ago by skaegi

For Eclipse Orion we're currently using a tweaked 1.6.1 with RequireJS and would _really_ like to move up to 1.8 but are blocked at the moment. We'd be happy to help out here but just want to make sure this is something the committers will work with us on.

comment:5 Changed 22 months ago by jburke

My first thought: that code is set up to want to know when all scripts have loaded and dom is ready. However, that should only be needed for indirect dependencies. I wonder if it is OK for non-dojo loaders to just have that code fire ready when the dom is ready, and just have the caveat for people using other loaders, make sure you have direct dependencies for things that need to be loaded.

Maybe that means the auto require stuff for the parser could not be supported in other loaders?

comment:6 Changed 22 months ago by neonstalwart

as someone who managed to get requirejs to work with dojo 1.6 at some point early on - i'd guess that you've got a few issues to work through to get dojo to work with any loader besides dojo's loader.

the main places that i've had issues with previously are:

  • dojo/i18n
  • dojo/text
  • the priority queue to implement dojo/ready

has anyone looked past this issue to see what else will be a problem?

also, what is missing/broken with dojo's loader that you would need to use another? maybe we should also be looking to address that?

another point... in dojo, you could use alias to replace the dojo/ready module with another implementation. if the other loaders offer a similar option (i think it might work to use paths) then you could replace that module with whatever implementation you want. i'm not saying that should be the conclusion of this... but it would move you forward to find what other issues might exist.

comment:7 Changed 22 months ago by rbackhouse

-- also, what is missing/broken with dojo's loader that you would need to use another? maybe we -- should also be looking to address that?

I thought one of the major selling points of AMD was interoperabilty. This does not help with winning over the AMD naysayers (and there are plenty). Its not so much what is missing but more what keeps getting added as proprietary API's.

comment:8 Changed 22 months ago by neonstalwart

i think proprietary APIs are the only way to innovate in a space that is still maturing. i used to lobby hard to have dojo be completely compatible with requirejs but after seeing things like packageMap that have come from proprietary APIs in dojo i came to the conclusion that complete interoperability was potentially a hindrance to moving AMD forward. by definition, innovation is doing something someone else isn't doing already.

i'll strongly defend dojo's proprietary API's for as long as they add value to users of dojo - this includes APIs that don't make sense outside of dojo but are needed in order to provide backwards compatibility. even requirejs has proprietary APIs not in other loaders (eg shim). would you ask james to remove this feature from requirejs because dojo doesn't have it? i wouldn't think you should - the feature is valuable to requirejs users and helps move AMD forward when library authors are reluctant to accommodate their own users who have adopted AMD.

comment:9 follow-up: Changed 22 months ago by peller

  • Cc dylan added

You can provide or use proprietary APIs, but when you do so, you're not using AMD. If these proprietary APIs are used in core classes, that's going to destroy interop, but it prevents innovation also. Take a look at Zazl. It provides tremendous value. It does something quite different from Dojo's loader, and it's a loader that may only be appropriate for certain use cases.. Maqetta swaps it in to do on-the-fly previews of user applications where a static dojo build doesn't make sense.

Interop with other libraries is key to Dojo's mission. We need to move away from a model where everyone has to adopt Dojo exclusively. We need to have smaller barriers for newcomers who wish to use Dojo who may already be using another library. There are things we will do well, and there are things other toolkits will do well. The loader choice should be arbitrary, and loaders can compete on their own merits.

comment:10 Changed 22 months ago by jburke

I would also separate extensions that deal with resolving string IDs to ones that add new methods on require/define. The string resolution ones are normally workable, where assuming proprietary require APIs (at least without doing a capability check) is more hazardous.

comment:11 in reply to: ↑ 9 Changed 22 months ago by neonstalwart

Replying to peller:

The loader choice should be arbitrary, and loaders can compete on their own merits.

i completely agree and my suggestion for 2.0 is that dojo should separate out it's loader and builder into their own repo(s) in order to even further emphasize that the toolkit is written in AMD and not in "dojo loader" and also, if the motivation is there for someone to promote it, the loader can compete on its own merits outside of dojo. unfortunately, until 2.0, we have to provide backwards compatibility with things that existed before AMD and so interoperability gets 2nd place.

that said, that doesn't necessarily preclude a fix for this specific issue. my first comment in this thread was mostly for the sake of seeing if anyone has looked into further issues - my other comments seem to have become the focus.

comment:12 follow-ups: Changed 22 months ago by jburke

I'm open to neonstalwart's suggestion of just mapping dojo/ready to another module as an acceptable alternative for now. I was hoping that just a conditional test for require.on was enough. Although I suppose there needs to be an else branch to at least trigger the callback when domready is available.

I'm also having other trouble building dojo though with r.js, there is something about knowing the right modules to include for a browser build that I cannot determine from executing the has plugin. I did a patch in #15444 to allow me to pass config to dojo/has, but that does not seem to be enough. More info in that other ticket, but I appreciate any pointers. I'm usually in the dojo-meeting irc channel when i'm on IRC.

comment:13 in reply to: ↑ 12 Changed 22 months ago by neonstalwart

Replying to jburke:

I'm also having other trouble building dojo though with r.js, there is something about knowing the right modules to include for a browser build that I cannot determine from executing the has plugin. I did a patch in #15444 to allow me to pass config to dojo/has, but that does not seem to be enough. More info in that other ticket, but I appreciate any pointers. I'm usually in the dojo-meeting irc channel when i'm on IRC.

you might want to look at http://bugs.dojotoolkit.org/browser/dojo/util/trunk/build/plugins for the code that dojo actually executes during a build - it takes a different approach to r.js for plugins.

Last edited 22 months ago by neonstalwart (previous) (diff)

comment:14 Changed 22 months ago by rcgill

harrisreynolds, if you're able to reproduce the problem on trunk, please open a new ticket. (Just noticed that Bill's fixes went into trunk, not 1.8) An example would be especially helpful.

Last edited 16 months ago by peller (previous) (diff)

comment:15 Changed 22 months ago by bill

  • Summary changed from require.on does not exist in some non-dojo loaders to make dojo work with other AMD loaders

I changed the summary to be hopefully more descriptive.

And yes I think it is the stuff I mentioned in #14587, i.e. all the modules that are currently using dojo/ready.

I'd be happy to review patches but I just don't have time to work on this myself.

Hopefully you could get most of the way there by making modules use dojo/domReady! when the dojo loader is in async mode (or when we are using a different AMD loader). Something like:

define([
         ...
         has("async") ? "dojo/domReady!" : "dojo/ready"
], function(...

(Or alternately have a flag to dojo/domReady! to return instantly when the loader is in sync mode, something like dojo/domReady!asyncOnly)

BTW, there's no issue with the parser's auto-require feature, it was just an issue with the optional parseOnLoad:true djConfig parameter. There's also a problem with dojox.layout.ContentPane's executeScripts feature, since it's hard to check what exact modules the embedded script may have tried to require. I don't think anyone cares though if those two features work with other AMD loaders.

Last edited 22 months ago by bill (previous) (diff)

comment:16 Changed 22 months ago by rcgill

In [29186]:

fixes amd config / allow foreign loaders to init has cache; thanks james burke; fixes #15444; refs #15616; !strict

comment:17 Changed 22 months ago by jburke

@neonstalwart: ah, thanks, so a different plugin is used for building. I'll see if I can adapt that to work in the requirejs loader. It may be a bit before I can fully try it out. I have a talk deadline and some family visiting.

@rcgill: on the "know when all modules complete". It is always worth bringing it up on amd-implement to see if others are interested in standardizing something to indicate "modules are loading", "all modules finished loading/executing".

I had something in requirejs 1.0 that the domReady plugin could attach to, but removed it in 2.0 as part of trying to consolidate the code, and I figured I would wait to see if people really needed, and if so, look at adding it back then. Particularly since when we talked about it before, ideally this is not needed -- use direct dependencies only. It is also hard for me to see how an ECMAScript module approach would try to allow for it, at least by default without making a custom module loader instance.

comment:18 Changed 22 months ago by bill

  • Milestone changed from tbd to 1.8.1
  • Owner rcgill deleted
  • Status changed from reopened to assigned

As per the meeting we won't hold up 1.8 for this, but hopefully put into a 1.8.1.

Erasing rawld's name though since there's no loader bug, just limitations in the dojo code.

comment:19 Changed 22 months ago by jamesthomas

  • Cc jamesthomas added

comment:20 Changed 22 months ago by peller

  • Cc rbackhouse added

comment:21 Changed 22 months ago by bill

  • Cc rcgill added

Rawld, as a hack to quickly support this pre-2.0, what do you think about modifying dojo/ready so that when used with other AMD loaders, it runs the priority queue on the DOMReady event? I.E. don't wait for all modules to load. It could be confusing, but could also be expedient.

Another question: I know that dojo/domReady! doesn't work as a dependency when running in sync mode, but what if it's used by itself, i.e. within a module (inside of the define() factory function) there's some code like:

require(["dojo/domReady!"], function(){
    // guaranteed delayed until DOMReady even when djConfig="async:false"?
})

comment:22 Changed 21 months ago by bill

PS: not sure my first question is pertinent, since the ready() callbacks could execute in any order if the domReady! happened before the ready() calls. I mean that the order of execution would be dependent on which modules happened to load first, which is somewhat random.

comment:23 Changed 21 months ago by randyhudson

I'd like to see "dojo/domReady!" deprecated. In our environment we optimize out every other form of plugin so that modules get evaluated immediately (i.e., define(…) immediately resolves dependencies and invokes the callback function. Everything gets sent down in a single script with all the define statements in the correct order.

Abuse of AMD plugins to register for an event is the only case where we wouldn't be able to do this, although I've yet to come across any use of dojo/domReady! that couldn't be converted to the non-plugin form.

comment:24 Changed 21 months ago by bill

I suppose that's a good idea, assuming that there was a domReady() (or ready()) function that queued callbacks to be run in the order they were registered. That way, a ready() callback in a dependency would be guaranteed to be executed before the ready() callback in the module that depends on it.

comment:25 Changed 20 months ago by bill

  • Blocked by 15866 added

comment:2 Changed 20 months ago by bill

In [29505]:

Various DOH robot updates:

  • Don't include dijit code into page with robot test code; dijit should only be loaded in the content iframe.
  • Fix double scrollbar on tests (on IE8) caused by overflow:visible setting on <html>
  • Prefer dojo/domReady! to dojo/ready, in order to support other AMD loaders. However, dojo/ready is still needed for the outer frame to detect when the inner frame (with the content) has completed initializing.
  • Since robot depends on dojo, use domClass.add() and remove() to add dohRobot class to <html> node, to avoid getting a space character in the class string.


Fixes #15865, refs #15616 on trunk/ !strict.

comment:3 Changed 20 months ago by bill

In [29506]:

Various DOH robot updates:

  • Don't include dijit code into page with robot test code; dijit should only be loaded in the content iframe.
  • Fix double scrollbar on tests (on IE8) caused by overflow:visible setting on <html>
  • Prefer dojo/domReady! to dojo/ready, in order to support other AMD loaders. However, dojo/ready is still needed for the outer frame to detect when the inner frame (with the content) has completed initializing.
  • Since robot depends on dojo, use domClass.add() and remove() to add dohRobot class to <html> node, to avoid getting a space character in the class string.


Fixes #15865, refs #15616 on 1.8/ branch !strict.

comment:4 Changed 20 months ago by bill

In [29507]:

For dojo core and dijit, use require(dojo/domReady!?, cb) rather than ready(cb) so that dojo code can run against other dojo loaders.

Exceptions:

  • robot.initRobot() related code that checks when page inside of iframe has finished initializing
  • dijit-legacy-requires back-compat code for synchronous mode, which load modules that weren't explicitly required (ex: how dijit/_WidgetBase loads dojo/_base/manager)
  • dojo/parser's parseOnLoad: true support
  • dojo-config-require sync mode operation

This change depends on dojo/domReady! executing it's callbacks in the order they were registered. To ensure that client-defined callback code executes after the callbacks inside of dojo/dijit, client code should either continue using dojo/ready, or use a nested require(dojo/domReady!?, ...) inside of the original require that loads the modules:

require([...], function(...){
         require(["dojo/domReady!"], ...)
}

Further, when the client code needs to run the parser and then run another block of code, it should be done like:

require([..., "dojo/parser"], function(..., parser){
     parser.parse().then(...);
}

Viewport.js needed a setTimeout() to workaround bug #15866. It should probably be removed when that bug is fixed.

Refs #15616, #15866 !strict. Changes on trunk/.

comment:5 Changed 20 months ago by bill

skaegi etc. - can you pull the code from trunk and see if this change is enough to make other AMD loaders work for you?

comment:6 Changed 20 months ago by bill

PS: my parser example should have been:

require([..., "dojo/parser"], function(..., parser){
     require(["dojo/domReady!"], function(){
         parser.parse().then(...);
     });
}

comment:7 Changed 20 months ago by skaegi

Thanks Bill -- sure.

comment:8 Changed 20 months ago by randyhudson

Replying to bill:

PS: my parser example should have been:

require([..., "dojo/parser"], function(..., parser){
     require(["dojo/domReady!"], function(){
         parser.parse().then(...);
     });
}

Please avoid using plugins when there is no reason to do so. Our AMD loader doesn't support plugins because we optimize away all use of plugins on the server. Instead:

require([..., "dojo/parser", "dojo/domReady"], function(..., parser, domReady){
     domReady(function(){
         parser.parse().then(...);
     });
}

Which is equivalent, more efficient, and works with loaders that only support server-side/build-time plugins.

comment:9 follow-up: Changed 20 months ago by bill

I didn't realize that domReady() could be used as a function, rather than a plugin, but looks like you are right. I'll revert my original checkin and do it that way instead.

comment:10 Changed 20 months ago by bill

In [29511]:

Use domReady() as a function rather than as a plugin, so dependencies can be fully resolved on the server.

This required a few changes to dojo/ready and dojo/domReady though:

  • for dojo/domReady, needed to make sure that tasks were executed in the order they were registered, including tasks registered after the DOMContentLoaded event but before the queue of tasks has been completed
  • delay executing dojo/ready callbacks until all tasks registered via domReady() complete, in order to insure that all the dojo initialization code runs before user defined code


Refs #15616 !strict

comment:11 Changed 20 months ago by bill

  • Milestone changed from 1.8.1 to 1.9

Note that [29511] broke _testCommon.js: URL's like dijit/tests/layout/test_TabContainer.html?theme=tundra don't display correctly anymore.

The changes above, plus the discussion to support dojo/domReady! for use with widgets seem a bit dangerous to roll into a point release, and contrary to the idea of point releases, so I'm not going to backport these changesets. If someone else wants to take responsibility for backporting and supporting the feature to 1.8, they can.

comment:12 follow-ups: Changed 16 months ago by harrisreynolds

Has there been any resolution to this issue? I'd like to use the Dojo charting components within a RequireJS application and I'm getting the about how the load/require object "has no method 'on'".

If there isn't a resolution, is there a workaround? A tweak I can make to either require or dojo to get this use case to work?

Thanks,

-- Harris

comment:13 Changed 16 months ago by bill

  • Resolution set to fixed
  • Status changed from assigned to closed

AFAIK it's all working. I asked @skaegi to try it 4 months ago (see above comments) but didn't get a response, so I'll just assume it's working.

comment:14 Changed 16 months ago by peller

harrisreynolds, if you're using the 1.8 release and you still have an issue, please open a new ticket. An example would be especially helpful.

comment:15 Changed 16 months ago by bill

The fix is only in trunk though, so you should try against the trunk code.

comment:16 Changed 16 months ago by ry4n

With the latest code in trunk require.on still throws an error for me when used with RequireJS.

dojo\dojo - at revision: 30255
dojo\dijit - at revision: 30255
At revision: 30255

TypeError: require.on is not a function
http://localhost:50001/js/vendor/dijit/_Widget.js?bust=v001
Line 130

onMouseDown() http://localhost:50001/js/vendor/dijit/_Widget.js?bust=v001 (line 130)

comment:17 Changed 16 months ago by bill

There's no require.on() call in _Widget.js. Are you running on a build or something? Or maybe that's just the debugger giving confusing messages.

Anyway, the one require.on() call I see in the current code, besides strangely in dojox/app, is in dojo/ready, which isn't meant to be portable to other AMD loaders.

I'm guessing you aren't using dojox/app?

But also, you shouldn't have any calls to dojo.ready() either, because the code in trunk was changed to use dojo/domReady! exclusively. So not sure what's going on with you getting this error.

comment:18 in reply to: ↑ 12 Changed 16 months ago by randyhudson

Replying to harrisreynolds:

is there a workaround? A tweak I can make to either require or dojo to get this use case to work?

Thanks,

-- Harris

Harris, I've implemented my own AMD loader, and I had to use the following stub:

	require.on = function ready(unused, callback) {
		callback();
	};

comment:19 Changed 16 months ago by ry4n

You are right. Firebug was giving a bad stacktrace. Webkit accurately points the finger at dojo/ready. I was trying to use dojo/parser's .parse() method to render bordercontainer/contentpane from existing markup which leads to this error.

This is off topic but is there some other way to use bordercontainer from existing markup?

comment:20 Changed 16 months ago by bill

Oh, for the parser thing, just stop setting parseOnLoad: true. Instead do:

require([... "dijit/layout/BorderContainer", "dojo/parser", "dojo/domReady!],
function(..., BorderContainer, parser){
    parser.parse();
});

comment:21 Changed 16 months ago by ry4n

I do have parseOnLoad set to false. I added domReady! but still get the require.on error. If there is somewhere else better to discuss this please let me know. I created a post on stackoverflow. http://stackoverflow.com/questions/14074313/using-dijit-without-dojo-parse

comment:22 Changed 16 months ago by bill

OK, then you need to use the debugger and figure out what's going on. AFAICT the ready() call only fires if parseOnLoad is set to true. The code is simple:

if(config.parseOnLoad){
	ready(100, parser, "parse");
}

comment:23 Changed 15 months ago by bill

FYI, I'm working on a branch of dojo and dijit that's actually (successfully) running against a different loader, see https://github.com/wkeese/dojo/tree/2.0 and https://github.com/wkeese/dijit/tree/2.0.

comment:24 Changed 15 months ago by jburke

bill: neat! I am also curious to try the r.js optimizer as I think from last time I tried a couple months ago that was difficult to do given way loader plugins are used and loaded in the dojo builder. Not surprising as that has been the weakest part of the AMD APIs across AMD implementations, but would be good to sort out, at least for a 2.0 if not in 1.x.

I have been meaning to dive into it more to give more meaningful, targeted feedback, but the day job has been eating more time lately.

comment:25 Changed 14 months ago by bill

In [30794]:

remove lingering reference to dojo/ready, refs #15616

comment:26 Changed 12 months ago by fditw

What about dojox using dojo/ready all over the place?

comment:27 Changed 12 months ago by csnover

  • Priority changed from undecided to blocker
  • Resolution fixed deleted
  • Status changed from closed to reopened

dojo/ready needs to fall back to a reasonable behaviour for other spec-compliant AMD loaders that do not support the Dojo 1 loader’s custom micro event API rather than just blindly assuming it is there and crashing. Anything that uses dojo/fx or dojo/parser does not work right now.

Last edited 12 months ago by csnover (previous) (diff)

comment:28 Changed 12 months ago by csnover

  • Owner set to csnover
  • Resolution set to fixed
  • Status changed from reopened to closed

In [31368]:

Fix dojo/ready to not blindly make calls to methods that might not exist on the require object. Fixes #15616.
!strict

comment:29 Changed 12 months ago by csnover

Most of the time, ready is used to continue to support discouraged coding methods. In dojo/fx, it is used in the sync path to load an extra fx module. In the parser, it’s used to allow parseOnLoad. Most of the rest of the time it’s just used as a DOM-ready execution queue. At worst, with the patch in [31368], we are no longer any more broken in other AMD loaders than we already were. It does not affect users that use the Dojo loader in any way since it has the methods that are tested for.

comment:30 Changed 12 months ago by csnover

In [31369]:

Fix dojo/ready to not blindly make calls to methods that might not exist on the require object. Fixes #15616.
!strict
Backport to 1.8

Note: See TracTickets for help on using tickets.