Opened 7 years ago

Closed 7 years ago

#16594 closed defect (fixed)

One of the MVC doh tests is failing after a change to dijit/_PaletteMixin.js

Reported by: Ed Chatelain Owned by: Ed Chatelain
Priority: undecided Milestone: 1.9
Component: DojoX MVC Version: 1.8.3
Keywords: Cc:
Blocked By: Blocking:

Description

After a change to _PaletteMixin.js this MVC test is failing: dojox/mvc/tests/doh/doh_mvc_form-kitchensink.html

dojox/mvc/at pulls in "dojox/mvc/_atBindingExtension", which I guess is the key.  If I require "dojox/mvc/_atBindingExtension"  before "dijit/ColorPalette" it works.  _atBindingExtension sets up the monkey patches dijit._WidgetBase for the mvc binding support, so I guess it makes sense that it would have to be done first, but I am not sure why it was working before the recent change to _PaletteMixin.

This was discussed with wildbill and asudoh, wildbill said the following: "The C3MRO code in dojo.declare(), converting multiple inheritance into single inheritance, is tricky, and I can't explain it completely.   But probably _WidgetBase is now getting used as a mixin rather than being directly in the prototype chain.   In other words, dojo.declare() making a copy of _WidgetBase when it creates subclasses.   So this line (from atBindingExtension) doesn't affect classes that have already been declared:"

asudoh responded: "Thanks Bill for more insights. As Bill mentioned, C3MRO logic seems to be the culprit, ending up with duplicate incorporation of _WidgetBase in the prototype chain (C3MRO code has some logic to avoid that, but seems not successful in dijit/ColorPalette case). Attached the code that demonstrates the root cause. I'm not sure if we can fix it (I'm guessing not...). So, as Ed mentioned, guiding app developers to make sure dojox/mvc/at is imported earlier sounds the best solution for now."

So I will update the testcase to move to require for dojox/mvc/at higher in the list.

Change History (7)

comment:1 Changed 7 years ago by Ed Chatelain

Milestone: tbd1.9
Status: newassigned

comment:2 Changed 7 years ago by Ed Chatelain

Resolution: fixed
Status: assignedclosed

In [30396]:

fixes #16594. fix tests to be sure dojox/mvc/at is early in the require list since that may be necessary for working with some widgets, like ColorPalette?. Also adding some new tests for diff controllers. !strict

comment:3 Changed 7 years ago by ben hockey

i wanted to make sure you realize that the patch you applied is still "broken". the deps are all loaded asynchronously so just moving them further up the list doesn't really properly fix the problem - you just get lucky more often.

overall there is still a general design flaw - patching prototypes after they have been subclassed/mixed in is not guaranteed to work when c3mro is in play. this has been one of my concerns with dojox/mvc from the beginning - it depends on patching prototypes and this approach is not guaranteed to work.

the only complete fix is that you have to explicitly fully load dojox/mvc/at before loading *any* widgets that depend on it's side effects. this is something that only a user can control and results in forcing the user to trigger the parser manually rather than using parseOnLoad: true (or don't use the parser at all).

require(['dojox/mvc/at', 'dojo/parser'], function (at, parser) {
    require(['dijit/ColorPalette'], function (ColorPalette) {
        // now it will always work - guaranteed
        parser.parse();
    });
});

advising users to move dojox/mvc/at further up the dependency list is probably not the best idea because it shows that we lack a full understanding of our own module system.

comment:4 Changed 7 years ago by bill

Resolution: fixed
Status: closedreopened

@neonstalwart, thanks for catching that, I'm gonna reopen this so it doesn't fall through the cracks.

BTW, I think another way to explain this is that declare([a, b, c], ... may create the new class using *copies* of a, b, and/or c.

comment:5 Changed 7 years ago by cjolif

Version 0, edited 7 years ago by cjolif (next)

comment:6 Changed 7 years ago by Ed Chatelain

In [30883]:

refs #16594. Support for per-widget MVC extension. !strict Thanks Akira Sudoh (IBM, CCLA).

comment:7 Changed 7 years ago by Ed Chatelain

Resolution: fixed
Status: reopenedclosed

With the code change above it is no longer necessary to move dojox/mvc/at further up the dependency list, the dojox/mvc/tests/doh/doh_mvc_form-kitchensink.html was updated [30922] move it back down.

Note: See TracTickets for help on using tickets.