Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#14808 closed defect (fixed)

dojo/require! no longer allows callback to happen (production build + legacy modules)

Reported by: Karl Tiedt Owned by: Rawld Gill
Priority: undecided Milestone: 1.7.3
Component: Loader Version: 1.7.2rc1
Keywords: Cc: cjolif
Blocked By: Blocking:

Description

changeset [27781] adds a traverse function to the _loader.js code, prior to this function existing dojo/require! in builds that contain legacy code worked properly

After this changeset, the callback for any defined() that includes the require! plugin no longer ever resolves its callback.

Attachments (2)

loader-patch (642 bytes) - added by Rawld Gill 7 years ago.
possible soln to problem
loader-patch.2 (4.5 KB) - added by Rawld Gill 7 years ago.
improved patch to solve problem

Download all attachments as: .zip

Change History (30)

comment:1 Changed 7 years ago by cjolif

Cc: cjolif added

comment:2 Changed 7 years ago by Rawld Gill

In [27905]:

added yet another test to show dojo/require plugin works; refs #14808

comment:3 in reply to:  description ; Changed 7 years ago by Rawld Gill

Replying to ktiedt:

Maybe you can use the code in [27905] to show how the loader is failing. I'm at a loss of how to proceed otherwise since I don't know of any specific test case where it fails.

comment:4 in reply to:  3 Changed 7 years ago by Karl Tiedt

Replying to rcgill:

Its true, a simple test works... but this doesnt change the fact that the original changeset mentioned breaks a real application due to some change relating to [27781] - sadly I cant provide that as a test case - its much too large and also can't be publicly released. I will try and catch where the error occurs, but at this point, it's not going in this release so its not exactly a dire time crunch either.

comment:5 Changed 7 years ago by Karl Tiedt

I traced our problem to checkComplete() calling execModule(module) with a module that has no .def property... however I was unable to determine *WHY* this is the case... and we cannot afford to troubleshoot farther on our own time for something that is "backwards compatible"

I'll continue trying to demonstrate a simpler test case as new ideas, but w/o better understanding of what the loader/builder code does in those areas... diving in and finding it has proven practically impossible.

comment:6 Changed 7 years ago by Karl Tiedt

Summary: dojo/require! no longer allows callback to happendojo/require! no longer allows callback to happen (production build + legacy modules)

comment:7 Changed 7 years ago by Rawld Gill

Resolution: worksforme
Status: newclosed

Tests and other users indicate this works. Since you've decided you can't show it doesn't, I'm closing this ticket. It is just as possible that you are doing something irrational in your code that just happened to work in prior versions by accident as it is that there is something wrong with the new code. I"m not saying the code is absolutely correct, only that there is no way for us to fix an error we can't see.

comment:8 Changed 7 years ago by Karl Tiedt

Can you at least detail what the changes in [27781] changed in terms of behavior -- the build/load code is not simple to dive into, that would at least give us something to work on in terms of figuring out what was happening prior to that changeset and what is being done after that changeset...

If that info can be clearly expressed, it would be greatly appreciated.

comment:9 Changed 7 years ago by Manuel Irrschik

We are currently facing the same problem here. Our first investigations also led to the fact, that every module containing "dojo/require!" in the dependencies does not resolve (the factory is not called).

Here's an example (legacy module, wrapped by build app):

//>>built
// wrapped by build app
define("card/LoginInterface", ["dijit","dojo","dojox","dojo/require!dijit/layout/_LayoutWidget,dijit/_TemplatedMixin,dijit/_WidgetsInTemplateMixin"], function(dijit,dojo,dojox){
dojo.provide('card.LoginInterface');

dojo.require('dijit.layout._LayoutWidget');
dojo.require('dijit._TemplatedMixin');
dojo.require('dijit._WidgetsInTemplateMixin');

dojo.declare('card.LoginInterface',
   [ dijit.layout._LayoutWidget, dijit._TemplatedMixin, dijit._WidgetsInTemplateMixin ],
{
  [...]
});

});

But (here it's getting strange for me): If we explicitly require each of the modules using dojo.require('...') before the "define" statement (for testing purposes) the factory gets called properly.

We are still looking at this, but as stated above, the load/build system is not that easy to understand. We are also trying to break it down to a simple test case, if we have any news we will post it here.

(this is just to inform that not only one person is having this kind of problem...)

comment:10 Changed 7 years ago by Manuel Irrschik

short test - undoing the changes of [27781] - working again (in fact only the line in the function checkDojoRequirePlugin function need to be changed from

return !traverse(module);

to

return module.injected!==arrived && !module.executed;

although this is a workaround for us (at least now) this is no reals solution... still trying to figure out why exactly this happens...

comment:11 Changed 7 years ago by Manuel Irrschik

i was finally able to produce a test-case:

http://dl.dropbox.com/u/37069753/14808_testCase.tar.gz

the test-case is based on dojo-release-1.7.2 (just downloaded for this test-case, not modified).

test1 fails, test2 and test3 are working.

Short explanation: When loading a module which contains a dojo/require! statement in its define statement, and one of the required modules contains a dojo/require! statement in its define too it fails.

(hope it is understandable... but just look at the test-case)

i do not think it is a user error, is it? we had no time to convert our modules to AMD yet so all of our modules are legacy modules and get wrapped with define(["dijit","dojo","dojox","dojo/require!..."] ... by the build tool

comment:12 Changed 7 years ago by Karl Tiedt

Resolution: worksforme
Status: closedreopened

reopening in light of manii's test case, I'm glad you had better luck narrowing it down!

comment:13 in reply to:  8 Changed 7 years ago by Rawld Gill

Replying to ktiedt:

Can you at least detail what the changes in [27781] changed in terms of behavior -- the build/load code is not simple to dive into, that would at least give us something to work on in terms of figuring out what was happening prior to that changeset and what is being done after that changeset...

If that info can be clearly expressed, it would be greatly appreciated.

http://dojotoolkit.org/reference-guide/loader/legacy.html#loader-legacy describes some of the loader's algorithms. It is hard/complex, and I understand you may dislike that document. I wish it wasn't so. But there are a lot of code paths requried to impl back compat with an amd loader in minimal code without maintaining three loaders and two build systems (remember, the legacy loader was two loaders alone).

comment:14 in reply to:  11 Changed 7 years ago by Rawld Gill

Status: reopenedassigned

Replying to manii:

i was finally able to produce a test-case:

Thanks, looking into this now.

Changed 7 years ago by Rawld Gill

Attachment: loader-patch added

possible soln to problem

comment:15 Changed 7 years ago by Rawld Gill

I've attached a patch; please give it a try. I need to add a test case and double check that this doesn't break other code.

comment:16 Changed 7 years ago by Karl Tiedt

Looks like it works here, running through our test pages, but the fact I can load them is a good sign! -- now only loader issue is i18n.js still loads from disk via XHR instead of from built file.

Thanks Rawld

comment:17 Changed 7 years ago by Manuel Irrschik

Seems to work here too, I'm building a test release for further testing, but looks promising.

Thanks!

Changed 7 years ago by Rawld Gill

Attachment: loader-patch.2 added

improved patch to solve problem

comment:18 Changed 7 years ago by Rawld Gill

while i believe my first fix will work for most cases, I've added a different patch that should handle some additional edge cases now that i understood the issue better.

ktiedt, manii: it would be great if you could try this second patch and give me some feedback...thanks!

comment:19 in reply to:  18 ; Changed 7 years ago by Karl Tiedt

Replying to rcgill:

while i believe my first fix will work for most cases, I've added a different patch that should handle some additional edge cases now that i understood the issue better.

ktiedt, manii: it would be great if you could try this second patch and give me some feedback...thanks!

This fix continues to work for our build.

comment:20 in reply to:  19 Changed 7 years ago by Manuel Irrschik

Replying to ktiedt:

Replying to rcgill:

while i believe my first fix will work for most cases, I've added a different patch that should handle some additional edge cases now that i understood the issue better.

ktiedt, manii: it would be great if you could try this second patch and give me some feedback...thanks!

This fix continues to work for our build.

same here, working fine!

comment:21 Changed 7 years ago by Kenneth G. Franqueiro

Milestone: tbd1.7.3

Marking this as 1.7.3 as it sounds like it absolutely should go into the next patch. Also got a report from neekfenwick on IRC that this resolved the same problem for him as well.

comment:22 Changed 7 years ago by Rawld Gill

In [28050]:

change dojo/require resolution algorithm to not consider dojo/require and dojo/loadInit modules; refs #14808; !strict

comment:23 Changed 7 years ago by Rawld Gill

In [28052]:

backport [28050]; refs #14808; !strict

comment:24 Changed 7 years ago by Rawld Gill

In [28357]:

added loader test; refs #14808

comment:25 Changed 7 years ago by Rawld Gill

Resolution: fixed
Status: assignedclosed

comment:26 Changed 6 years ago by Bill Keese <bill@…>

In e6c95fea9087099b73e735f00ac4fcc6cde0eff0/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:27 Changed 6 years ago by bill

Whoops, above checkin was for #14828, not this ticket.

comment:28 Changed 6 years ago by Bill Keese <bill@…>

In 3daa1762e41b3050ea1327baaf0a59d62383e2e7/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.