Opened 8 years ago

Last modified 3 years ago

#13769 assigned defect

[patch][cla]Circular dependency in dojo/dom-attr

Reported by: Richard Backhouse Owned by: bill
Priority: high Milestone: 1.15
Component: HTML Version:
Keywords: Cc: Rawld Gill, James Burke
Blocked By: Blocking:

Description

I'm seeing a circular dependency in dojo/dom-attr. Here is the dependency chain :

dojo/dom-attr->dojo/dom-prop->dojo/dom-construct->dojo/dom-attr

Attachments (4)

fix-circular-deps-in-dom-related-modules.tar.gz (3.5 KB) - added by zynevich 8 years ago.
Refactoring to broke circular deps
no-circular-dom-modules.diff (3.6 KB) - added by zynevich 8 years ago.
Proposed fix for version 1.7.1
no-circular-dom-modules-updated.patch (3.2 KB) - added by bill 7 years ago.
updated patch to apply cleanly to trunk
no-circular-dom-modules-updated2.patch (9.0 KB) - added by bill 7 years ago.
updated patch to return a hash from each file (like other modules), rather than using exports; it exposed the problem that dom-attr still depends on dom-prop

Download all attachments as: .zip

Change History (48)

comment:1 Changed 8 years ago by bill

Component: CoreHTML
Owner: set to Eugene Lazutkin

This is expected, that's why those modules use "exports" to set their "return" value.

Are you seeing a bug, or just wanted to mention that the dependencies are circular?

If the code you are looking at has a "return ..." statement at the end, then you need to get the latest code.

comment:2 Changed 8 years ago by Richard Backhouse

The modules load however in an optimized environment where the modules are loaded via a single resource and they is in their correct dependency order the dojo AMD loader throws "Error: multipleDefine ". The modules are also double loaded via additonal HTTP requests. I'm not sure if/how the dojo build handles this.

I would think that removing the dependency from the "define" array arg and using require to load it would avoid the issue. Something like

define(["exports", "require", "./_base/sniff", "./_base/lang", "./dom", "./dom-style"],

function(exports, require, has, lang, dom, style){

and then in

exports.set = function setAttr(/*DOMNode|String*/node, /*String|Object*/name, /*String?*/value){

Add

var prop = require("./dom-prop");

comment:3 Changed 8 years ago by bill

Component: HTMLBuildSystem
Owner: changed from Eugene Lazutkin to Rawld Gill

Hmm, well can you give a test case, like your build profile? We've tested the standard profile which I assume loads all of the DOM code, and it seems to run fine.

comment:4 Changed 8 years ago by Rawld Gill

Status: newassigned

comment:5 Changed 8 years ago by Rawld Gill

I'm not sure of the status of this. In #13410, I fixed an error that incorrectly caused multiple defines. Could you please retry your test case, and, if it fails, try to reduce and publish it. Thanks!

comment:6 Changed 8 years ago by Rawld Gill

Milestone: tbd1.7

comment:7 Changed 8 years ago by Richard Backhouse

The concern I have is that there is a hard coded circular dependency in the core dojo code. While circular dependencies should certainly be tolerated I feel they should not be encouraged, especially in such a core piece of code.

I'm guessing that the reason this has not come up as an issue elsewhere is because the dojo build tool places all the modules in require({cache:{.... and the loader pulls from there. Other AMD loaders and optimizer/build tools that simply concatenate modules together will produce something that results additional loads via http/xhr. That's what I'm seeing when using the dojo loader will a single concatenated resource.

comment:8 Changed 8 years ago by bill

Don't worry, we aren't encouraging circular dependencies. We only did it with the HTML code because it was unavoidable, at least given the desire to split the massive html.js module into multiple modules.

Since you didn't attach a test case I assume that means that everything is now working for you?

comment:9 Changed 8 years ago by Richard Backhouse

The problem is still there. Here is a link to a testcase http://www.zazl.org/downloads/amdtest.zip. It's too big to upload to trac (750k). It loads a test.js file that contains the Dojo loader + all the required dependencies in their correct order.

Unzip into a webserver and load http://localhost/amdtest/amdcalendar.html. You should see that http://localhost/amdtest/dojo/dom-attr.js is pulled in again even though its contained within test.js. (Note there are other scripts also loaded independently, however this is probably due to an AMD plugin loading them).

I have to ask why is it unavoidable ? Why can't the common code be pulled from dom-attr and pushed into a module that both dom-attr and dom-construct depends on ?

comment:10 Changed 8 years ago by bill

Certainly, it could be avoided like that, or by putting all functions into one file, or by putting each function into a separate file. But what we wanted was to have one file with all the attribute related methods, and one file with all the dom-node construction methods, etc. FYI this is from #9641.

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

comment:11 Changed 8 years ago by Rawld Gill

Owner: changed from Rawld Gill to Eugene Lazutkin
Status: assignednew

This is an issue with the html split, not the loader. Therefore, I'm transferring ownership to Eugene. My input is to not change anything for 1.7.

comment:12 Changed 8 years ago by bill

Cc: Rawld Gill added

So the test case doesn't demonstrate an issue with the loader? (Of course, I have no desire to look at a 750K test case, maybe we should demand something of a reasonable size.)

comment:13 Changed 8 years ago by Richard Backhouse

Rawld is right. The problem lies with the dom code. The loader, when used in this way, reports "Error: multipleDefine ".

src: dojoLoader test.js (line 1586) info: Object { pid="dojo", mid="dojo/dom-attr", pack={...}, more...} test.js (line 1586)

If/When? requirejs loads this code via its optimizer I suspect it's going to also double load the module via an additional script injection.

comment:14 Changed 8 years ago by bill

Huh, what's wrong with the DOM code?

comment:15 Changed 8 years ago by Richard Backhouse

If there is going to be no plan to remove this circular dependency then I would like to request that the code is at least made more robust. While the dojo loader may be tolerant of this code other AMD loaders may not be so forgiving.

I would like to suggest that an additional check be added to the dojo/dom-construct method "exports.create"

if (attr === undefined) {

attr = req("./dom-attr");

}

if(attrs){ attr.set(tag, attrs); }

where there "req" variable is a passed in "require" object :

define(["exports", "./_base/kernel", "./_base/sniff", "./_base/window", "./dom", "./dom-attr", "./on", "require"],

function(exports, dojo, has, win, dom, attr, on, req){

This will ensure that the attr object is resolved consistently.

Note this is one of the options recommended by the requirejs docs concerning circular dependencies :

http://requirejs.org/docs/api.html#circular

Last edited 8 years ago by Richard Backhouse (previous) (diff)

comment:16 Changed 8 years ago by bill

Cc: James Burke added

@rbackhouse - the last paragraph of that link says:

If you are familiar with CommonJS modules, you could instead declare exports as a dependency to create an empty object for the module. By doing this on both sides of a circular dependency, you can then safely hold on to the the other module via a function argument.

Isn't that what we are already doing? dom-construct.js is:

define(["exports", "./_base/kernel", "./_base/sniff", "./_base/window", "./dom", "./dom-attr", "./on"],
		function(exports, dojo, has, win, dom, attr, on){
...
exports.toDom = ...
...

and likewise for the other dom-* modules.

comment:17 Changed 8 years ago by James Burke

Just confirming that Bill is correct -- using exports should allow circular dependencies to work. Doing a quick look at the circular dependency mentioned in this bug, it seems like all of the property references on the exports objects are only referenced inside functions, not as part of module exports construction, which is good. If a reference is used as part of module reference construction is used, then it would fail, but so would an explicit require() call.

comment:18 Changed 8 years ago by Richard Backhouse

I missed the use of exports on both ends of the circular ref. I guess this can be closed if there is no plan to restructure the code to remove it.

comment:19 Changed 8 years ago by bill

Resolution: invalid
Status: newclosed

OK then, closing this ticket, assuming that if Eugene had any plans to change the design he would have commented on it sometime in the past four months.

Marking the ticket as "invalid" since it's implying that the circular dependency is a bug.

comment:20 Changed 8 years ago by zynevich

Resolution: invalid
Status: closedreopened

Though circular dependencies is not a bug, it is a feature, I propose to broke this circle, because it is not a good practice. Another issue with circular deps is that they should be treated specially and produce extra "toil" on the AMD-loader. This toil in not a series of heavy computations (though in case of tones of modules in complicated circular graph it might be so) but it challenges a level of accuracy of the loader's implementation. Currently because of this circle, require.js may not load Dojo from unoptimized code: http://groups.google.com/group/requirejs/browse_thread/thread/e5803a84feaf044d

I refactored vanilla dojo to broke this circular dependency, after that require.js loaded dojo 1.7.1 successfully, I attached changed files. (Of course the bug in require.js not in Dojo, but this change or similar may make Dojo better and more robust)

Changed 8 years ago by zynevich

Refactoring to broke circular deps

comment:21 Changed 8 years ago by bill

I can't tell what you changed from that tar file, can you attach a patch file (aka unified diff)?

comment:22 Changed 8 years ago by John Hann

+1. please fix (remove circular dependency)

Changed 8 years ago by zynevich

Proposed fix for version 1.7.1

comment:23 Changed 7 years ago by ben hockey

Component: BuildSystemCore
Keywords: needsreview added
Priority: highlow
Resolution: fixed
Status: reopenedclosed

comment:24 Changed 7 years ago by ben hockey

Resolution: fixed
Status: closedreopened

sorry - didn't mean to close

comment:25 Changed 7 years ago by ben hockey

#15131 is a duplicate of this ticket.

comment:26 Changed 7 years ago by Adam Peller

Priority: lowhigh
Summary: Circular dependency in dojo/dom-attr[patch][no cla?]Circular dependency in dojo/dom-attr

I've got a fairly simple use of dom-construct in Maqetta which is breaking because of this circular dependency. This could probably be solved with the workaround @rbackhouse suggests, but a refactor to avoid the circular dependency is obviously better. Can someone review @zynevich's fix? Does it require a CLA? (I'd be inclined to say no)

Last edited 7 years ago by Adam Peller (previous) (diff)

comment:27 Changed 7 years ago by bill

Adam, can you provide a test case (less than 750K)? As I said above, circular dependency is not a bug, so if something isn't working then that indicates a bug outside the DOM code. Perhaps in the sync loader. @rbackhouse's suggestion should not be necessary, as he himself said above in comment #18.

I'm not necessarily opposed to removing the circular dependency, but it would be good to solve the actual bug too, rather than sweeping it under the rug.

Note: After zynevich's patch, the dependencies between the dom* modules are:

_dom-prop-names --> no dependencies
dom --> no dependencies
dom-class --> "./dom"
dom-form --> "./dom"
dom-style --> "./dom"
dom-attr --> "./dom", "./dom-style",  "./_dom-prop-names"
dom-construct --> "./dom", "./dom-attr",
dom-geometry --> "./dom", "./dom-style"
dom-prop --> "./dom", "./dom-style", "./dom-construct", "./_dom-prop-names"

So it does resolve the circular dependency. The patch looks OK to me, although I had some trouble applying it so I'll upload an updated version.

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

Changed 7 years ago by bill

updated patch to apply cleanly to trunk

comment:28 in reply to:  27 ; Changed 7 years ago by Adam Peller

Replying to bill:

Adam, can you provide a test case (less than 750K)?

I do not have an isolated test case. Even with maqetta (trunk) the problem is intermittent, as it is related to the timing of other dojo code loading Dom-atrr

As I said above, circular dependency is not a bug, so if something isn't working then that indicates a bug outside the DOM code. Perhaps in the sync loader.

the circular dependency is not a bug in itself. But dojo/dom-construct references dojo/dom-attr without using an inline require() It uses the dependency list directly, resulting in an undefined reference., failing as @burke says it should.

FWIW I am seeing this with synch loading

@rbackhouse's suggestion should not be necessary, as he himself said above in comment #18.

It's still needed, unless we remove the hard dependency another way

I'm not necessarily opposed to removing the circular dependency, but it would be good to solve the actual bug too, rather than sweeping it under the rug.

Note: After zynevich's patch, the dependencies between the dom* modules are:

_dom-prop-names --> no dependencies
dom --> no dependencies
dom-class --> "./dom"
dom-form --> "./dom"
dom-style --> "./dom"
dom-attr --> "./dom", "./dom-style",  "./_dom-prop-names"
dom-construct --> "./dom", "./dom-attr",
dom-geometry --> "./dom", "./dom-style"
dom-prop --> "./dom", "./dom-style", "./dom-construct", "./_dom-prop-names"

So it does resolve the circular dependency. The patch looks OK to me, although I had some trouble applying it so I'll upload an updated version.

Version 0, edited 7 years ago by Adam Peller (next)

comment:29 in reply to:  28 Changed 7 years ago by bill

Replying to peller:

the circular dependency is not a bug in itself. But dojo/dom-construct references dojo/dom-attr without using an inline require() It uses the dependency list directly, resulting in an undefined reference., failing as @jburke says it should.

Huh? @jburke said that it should work, see comment:17

comment:30 Changed 7 years ago by bill

FYI, I filed #15137 to track the apparent loader problem that Adam saw in Maquetta. Although I didn't realize that he was referencing the DOM modules during load, from a plugin. Maybe that's not supposed to work.

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

comment:31 Changed 7 years ago by bill

@ rbackhouse - can you give a working link to your test case? The link you gave doesn't work.

comment:32 Changed 7 years ago by Richard Backhouse

Try again. It should be available again at http://www.zazl.org/downloads/amdtest.zip

comment:33 Changed 7 years ago by bill

Thanks. The link is working now but I only see dom-attr.js being loaded once. This is from the NET tab of firebug, with the JS sub-tab selected. It shows 15 things being loaded.

comment:34 Changed 7 years ago by Richard Backhouse

If you look at the included test.js which is a concatenated set of all the required modules dom-attr.js is included in there. It should not be loaded again which is what you see in the net tab.

comment:35 Changed 7 years ago by bill

@rbackhouse - OK, so test.js was generated from a build? If so, can you add the build profile and the instructions to use it, including the dojo version? Plus any auxiliary files needed. It sounds like your test case is really just three (source) files: test.js, Calendar.js, and a build profile, is that right?

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

comment:36 Changed 7 years ago by Richard Backhouse

From my comment 2:

"The modules load however in an optimized environment where the modules are loaded via a single resource and they are in their correct dependency order the dojo AMD loader throws "Error: multipleDefine ". The modules are also double loaded via additonal HTTP requests. I'm not sure if/how the dojo build handles this."

From my comment 7:

"I'm guessing that the reason this has not come up as an issue elsewhere is because the dojo build tool places all the modules in require({cache:{.... and the loader pulls from there. Other AMD loaders and optimizer/build tools that simply concatenate modules together will produce something that results additional loads via http/xhr. That's what I'm seeing when using the dojo loader will a single concatenated resource. "

I'm not sure that this demonstrating Adams issue.

comment:37 Changed 7 years ago by bill

Component: CoreHTML

comment:38 Changed 7 years ago by Rawld Gill

I don't see anything wrong with the HTML code. It is written to spec, and provably works. There are times that circles make the code more rational. It seems to me if Eugene thinks this is one, then that should be good enough.

@rbackhouse That a particular build design causes a different code path in built code compared to dev code indicates a nonoptimal design--particularly when there are other designs that don't have the problem. The dojo builder makes every effort to ensure that code executes the same whether built or unbuilt. I'm not saying this is 100% attainable, but that should be the goal.

The old builder did not do this, and it caused problems in some cases. Simple concatenation of modules with absolute mids as is done in some AMD builders is definitely not robust. For example, that format would be completely incapable of mapping namespaces.

In the final analysis, code that is written according to spec and works perfectly in a dev environment, but fails to load after a build seems to indicate a problem with the build strategy rather than the code.

comment:39 Changed 7 years ago by Adam Peller

Just the same, if the code can be easily refactored to avoid a circular dependency, I can't imagine why we shouldn't do it.

comment:40 Changed 7 years ago by bill

By popular demand, and since Eugene said it was OK, I was about to apply the patch above. But I hit problems because dojo/dom-attr::set() still depends on dojo/dom-prop:set()

if(forceProp || typeof value == "boolean" || lang.isFunction(value)){
        return prop.set(node, name, value);
}

Seems like the patch is insufficient? dom-attr --> dom-prop--> dom-construct (empty, toDom) --> dom-attr:

  • dom-attr::set() calls dom-prop::set()
  • dom-prop::set() calls dom-construct::empty() and dom-construct::toDom()
  • dom-construct::create() calls dom-attr::set()

The dependency could be addressed by moving dom-construct::create() to a separate file, but things are starting to get ugly.

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

Changed 7 years ago by bill

updated patch to return a hash from each file (like other modules), rather than using exports; it exposed the problem that dom-attr still depends on dom-prop

comment:41 Changed 7 years ago by Colin Snover

Milestone: 1.82.0

1.8 has been tagged; moving all outstanding tickets to next major release milestone.

comment:42 Changed 4 years ago by ankaggar

The provided patches do not remove all dependencies on dom-prop module i.e. dom-prop module is still getting used in dom-attr module in the set function as follows:

dom-attr.js

if(forceProp || typeof value == "boolean" || lang.isFunction(value)){
	return prop.set(node, name, value);
}

where "prop" in prop.set above refers to dom-prop module.

comment:43 Changed 4 years ago by dylan

Keywords: needsreview removed
Milestone: 2.01.12
Owner: changed from Eugene Lazutkin to bill
Status: reopenedassigned
Summary: [patch][no cla?]Circular dependency in dojo/dom-attr[patch][cla]Circular dependency in dojo/dom-attr

Not an issue for Dojo 2. That said, given how close Bill's patch is, we should probably fix for 1.12.

comment:44 Changed 3 years ago by dylan

Milestone: 1.131.15

Ticket planning... move current 1.13 tickets out to 1.15 to make it easier to move tickets into the 1.13 milestone.

Note: See TracTickets for help on using tickets.