Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9862 closed enhancement (fixed)

Promote dojox.lang.oo.declare to dojo.declare

Reported by: James Burke Owned by: Eugene Lazutkin
Priority: high Milestone: 1.4
Component: Core Version: 1.3.2
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by Eugene Lazutkin)

dojo.lang.oo.declare has:

  • fixes for speed/behavior, resolving super chains.
  • allows anonymous "class" declarations (not requiring a string for the first argument)
  • more configurable options on automatic super call chaining.

Want to provide this as the default dojo.declare.

Attachments (2)

declare-old.js (9.2 KB) - added by Eugene Lazutkin 10 years ago.
the old version of dojo.declare, can be used for testing and as a reference
testDeclareExtend.html (690 bytes) - added by sjmiles 10 years ago.
Test case for dynamic (post-declare) class extension

Download all attachments as: .zip

Change History (85)

comment:1 Changed 10 years ago by Eugene Lazutkin

Status: newassigned

comment:2 Changed 10 years ago by Eugene Lazutkin

(In [20068]) lang.oo: simplifications and performance improvements across the board, !strict, refs #9862.

comment:3 Changed 10 years ago by Eugene Lazutkin

Previous commits: [17126], [19482], [19483], [19487], [20032], [20036], [20037], [20038], [20041].

The rest (e.g., different algorithms and tweaks) is in my private repository.

comment:4 Changed 10 years ago by Eugene Lazutkin

(In [20088]) lang.oo: cleaning up oo.declare() to our coding standards, the previous version is preserved as declare-x, !strict, refs #9862.

comment:5 Changed 10 years ago by Eugene Lazutkin

(In [20089]) lang.oo: clean up and fixing the preamble bug, !strict, refs #9862, refs #9795.

comment:6 Changed 10 years ago by Eugene Lazutkin

(In [20126]) declare: committing the new version with updated inline documentation, and getInherited(), !strict, refs #9862, refs #9795.

comment:7 Changed 10 years ago by bill

The only failure I'm seeing in dijit is dijit/tests/layout/ContentPaneLayout.html. Haven't looked into what exactly is failing but it works before the switch over.

comment:8 in reply to:  7 Changed 10 years ago by Eugene Lazutkin

Replying to bill:

The only failure I'm seeing in dijit is dijit/tests/layout/ContentPaneLayout.html. Haven't looked into what exactly is failing but it works before the switch over.

These are links to dojo.declare:

They all use old dojo.declare (you can inspect files).

These are links to the test in question:

The test from 9/15 works for me in FF, other two fail --- all three use old dojo.declare.

The test was changed on 9/15 and most probably at that time some bugs were introduced. Could you double-check that it works with the old dojo.declare? I cannot do it locally because the test requires PHP, and I don't have it on my web server.

Changed 10 years ago by Eugene Lazutkin

Attachment: declare-old.js added

the old version of dojo.declare, can be used for testing and as a reference

comment:9 Changed 10 years ago by bill

Yes, I used the old version of dojo.declare() as a reference, that's how I know that it's the new dojo.declare() where it breaks. Even triple checked. I'm not sure what's up with the nightlies, perhaps just a timing race condition. Anyway the latest code works for me only if I rollback [20126].

The first failing test is "single resizable href", and the problem is that StackContainer.resize() calls this.inherited() which calls _LayoutWidget.resize(). There's a connection to that _LayoutWidget.resize() call which isn't getting called:

dojo.connect(dijit.layout._LayoutWidget.prototype, "resize", function(){
	// this is the pointer to the widget, and arguments are newsize/curSize args to resize()
	var ary = layoutResizes[this.id];
	if(!ary){
		ary = layoutResizes[this.id] = [];
	}
	ary.push(arguments);
});

I don't know if you want to support connecting to prototype methods, but it was working before and I remember some other code somewhere that connects to methods on the prototype.

comment:11 Changed 10 years ago by Eugene Lazutkin

(In [20154]) dtl: minor fix --- "classes" should be functions, not objects, !strict, refs #9862.

comment:12 Changed 10 years ago by Eugene Lazutkin

(In [20155]) dojo.declare: minor refactoring, support for dynamically redefined methods, !strict, refs #9862.

comment:13 in reply to:  9 Changed 10 years ago by Eugene Lazutkin

Replying to bill:

[20155] should take care of dojo.connect on prototype methods problem. I can't test it myself, so please try to run it and see if it works for you.

Replying to dante:

[20156] should take care of the DTL bug. One of "classes" was an object with a prototype property. It worked before because the old version of dojo.declare didn't allow native objects. New version assumes that if "class" doesn't define the "constructor" property in the prototype, that the "class" itself (a function) is used to construct the object. Obviously it doesn't work with plain objects. The fix is trivial.

comment:14 Changed 10 years ago by Eugene Lazutkin

Sorry, [20156] is wrong. It should be [20154].

comment:15 Changed 10 years ago by Eugene Lazutkin

(In [20156]) dojo.declare: new test that uses 3 different forms of prototype method augmentation/replacement, !strict, refs #9862.

comment:16 Changed 10 years ago by Eugene Lazutkin

[20156] emulates Bill's case and it works now. Still we need to test the real thing.

comment:17 Changed 10 years ago by Eugene Lazutkin

(In [20157]) lang.oo: temporarily switching oo.declare tests to the new dojo.declare to verify that it works as expected, !strict, refs #9862.

comment:18 Changed 10 years ago by bill

Cool, ContentPaneLayout.html is working now, thanks.

comment:19 Changed 10 years ago by Eugene Lazutkin

(In [20158]) dojo.declare: more accurate work with inherited "raw classes", !strict, refs #9862.

comment:20 Changed 10 years ago by Eugene Lazutkin

(In [20160]) lang.oo: modified the benchmark to use the existing (new) dojo.declare), the old one is copied and modified to make this benchmark possible, !strict, refs #9862.

comment:21 Changed 10 years ago by Eugene Lazutkin

(In [20161]) declare: minor code reduction, !strict, refs #9862.

comment:22 Changed 10 years ago by Eugene Lazutkin

(In [20162]) dojo.declare: faster algorithm for C3 MRO, !strict, refs #9862.

comment:23 Changed 10 years ago by Eugene Lazutkin

(In [20163]) dojo.declare: a speed bust, !strict, refs #9862.

comment:24 Changed 10 years ago by Les

or rather speed boost :)

comment:25 Changed 10 years ago by bill

Could you check dijit/tests/_Templated-widgetsInTemplate.html? I'm getting a hang in the c3mro code. Thanks.

comment:26 in reply to:  24 Changed 10 years ago by Eugene Lazutkin

Replying to Les:

or rather speed boost :)

:-)

Replying to Bill:

Could you check dijit/tests/_Templated-widgetsInTemplate.html? I'm getting a hang in the c3mro code. Thanks.

Will do.

comment:27 in reply to:  25 Changed 10 years ago by Eugene Lazutkin

Replying to bill:

Could you check dijit/tests/_Templated-widgetsInTemplate.html? I'm getting a hang in the c3mro code. Thanks.

That test code inadvertently triggered a bug in my code. Here is how TestLayoutWidget is defined:

dojo.declare('TestLayoutWidget',
  [dijit._Widget, dijit.layout._LayoutWidget], {
...
});

Note dijit._Widget as the base, and dijit.layout._LayoutWidget as the mixin.

Here is how dijit.layout._LayoutWidget is defined:

dojo.declare("dijit.layout._LayoutWidget",
  [dijit._Widget, dijit._Container, dijit._Contained], {
...
});

As you can see the dijit._Widget is here again. While this structure is perfectly legal, it doesn't make much sense.

With the old dojo.declare dijit._Widget is going to be doubled. With the new one it is supposed to be collapsed, but I had a bug in my code.

I'll check in the fix shortly after running more tests to make sure I didn't break anything else while fixing the bug.

comment:28 Changed 10 years ago by bill

Ah, ok thanks! I'll fix that error in _Templated-widgetsInTemplate.html (it wasn't intended to include _Widget twice).

comment:29 Changed 10 years ago by bill

(In [20173]) TestLayoutWidget? (unintentionally) extended from _Widget twice. Refs #9862.

comment:30 in reply to:  29 Changed 10 years ago by Eugene Lazutkin

Replying to bill:

(In [20173]) TestLayoutWidget? (unintentionally) extended from _Widget twice. Refs #9862.

Another possible enhancement (does not matter in this particular case of course) is to change the code from this pattern:

declare("A", [B], {...});

to this:

declare("A", B, {...});

because it is a tiny bit faster --- it is handled as a special case.

Another tiny enhancement is to convert this code:

declare("A", [], {...});

to this:

declare("A", null, {...});

It is a special case too.

comment:31 Changed 10 years ago by Eugene Lazutkin

(In [20174]) dojo.declare: minor refactoring, !strict, refs #9862.

comment:32 Changed 10 years ago by Eugene Lazutkin

(In [20175]) Fixing the bug reported by Bill, switching to arrays for C3 MRO algorithm, minor speed tweaks, !strict, refs #9862.

comment:33 Changed 10 years ago by Nathan Toone

The following code used to work previously (I saw it on some example website at one point) but it now fails with the error "Mixin #0 is null":

dojo.declare("A", [null, B], {...});

The idea being you want to create a "double-mixin" - you don't want it to inherit directly from any class - but you want it to have another mixin's class mixed in.

comment:34 Changed 10 years ago by Nathan Toone

Another issue that is not working with these changes (but did work previously):

dojo.setObject("MyMixin"), {
  resize: function(){
    this.inherited("resize", arguments);
  }
}
var cp = new dijit.layout.ContentPane();
dojo.mixin(cp, MyMixin);
cp.resize();  // <- This gives an error because it can't find the "inherited" function to call

Again - this was based off an example that I had read somewhere - and it has worked until these latest changes.

comment:35 Changed 10 years ago by Nathan Toone

correction: the resize() call in the above example doesn't give an error - it just doesn't call the inherited function.

comment:36 in reply to:  33 ; Changed 10 years ago by Eugene Lazutkin

Replying to toonetown:

The following code used to work previously (I saw it on some example website at one point) but it now fails with the error "Mixin #0 is null":

That one should be changed in your code --- one more special case will bloat the code more (I already added almost 100 bytes to support inheritance from [] instead of null and [A] instead of A). Frankly I don't see any compelling reasons for "double-mixin".

Even if you do have a highly compelling reason for that (I can't see any), you can always do:

var A = function(){};
dojo.extend(A, B.prototype, {...});

comment:37 in reply to:  34 ; Changed 10 years ago by Eugene Lazutkin

Replying to toonetown:

Another issue that is not working with these changes (but did work previously):

While general dynamic mixing of methods is not supported directly (they don't have a proper meta-information for one), your example should work. Probably there is a bug in my code. I'll look into that.

comment:38 in reply to:  36 Changed 10 years ago by Nathan Toone

Replying to elazutkin:

That one should be changed in your code --- one more special case will bloat the code more (I already added almost 100 bytes to support inheritance from [] instead of null and [A] instead of A). Frankly I don't see any compelling reasons for "double-mixin".

OK - I'm fine with that as a solution.

comment:39 in reply to:  37 ; Changed 10 years ago by Nathan Toone

Replying to elazutkin:

While general dynamic mixing of methods is not supported directly (they don't have a proper meta-information for one), your example should work. Probably there is a bug in my code. I'll look into that.

I can write up a test case, if you like.

comment:40 in reply to:  39 ; Changed 10 years ago by Eugene Lazutkin

Replying to toonetown:

I can write up a test case, if you like.

The current code handles two dynamic things:

  1. One of the prototypes is modified dynamically, and the new method calls this.inherited() directly. Obviously it needs to supply a name.
  2. One of the prototypes is modified dynamically, and the new method calls the old method, which may call this.inherited() (as it was programmed to do).

Apparently I missed the addition of a dynamic method directly on an instance. I'll add the support for it and it will work like this: it will call the first inherited method available effectively starting the chain of inherited calls from the beginning. If this is something you expect, I'll write a unit test myself, otherwise you should write it and explain how you expect it to work.

comment:41 in reply to:  40 Changed 10 years ago by Nathan Toone

Replying to elazutkin:

Apparently I missed the addition of a dynamic method directly on an instance. I'll add the support for it and it will work like this: it will call the first inherited method available effectively starting the chain of inherited calls from the beginning. If this is something you expect, I'll write a unit test myself, otherwise you should write it and explain how you expect it to work.

Yes - that is what I would expect. Basically, just being able to mix it into an existing instance.

comment:42 Changed 10 years ago by bill

(In [20195]) Don't use array when there's only one superclass. It's a tiny bit faster (and two less bytes of code...). Refs #9862 !strict.

comment:43 Changed 10 years ago by Nathan Toone

(In [20203]) Refs #9862 - add test case for mixing in new functions to an instance of a class - this test passes with revision [20154] (though the mutatedMethods test breaks in that revision) and breaks in [20155] (which fixes the mutatedMethods test) !strict

comment:44 Changed 10 years ago by Eugene Lazutkin

(In [20214]) dojo.declare: fixing a bug reported by toonetown, thx!, !strict, refs #9862.

comment:45 Changed 10 years ago by sjmiles

As discussed on dojo-dev, old declare added an 'extend' to function to declared constructors that added methods to a prototype and added helpful meta-data. Tried to make a patch to restore that functionality, but ran into this problem:

dojo.declare("A", null, {});

// simulate A.extend
A.prototype.id = function() {
		log("A.id");
};
A.prototype.id.nom = "id";
		
dojo.declare("B", A, {
	id: function() {
		log("B.id");
		this.inherited(arguments);
	}
});
		
b = new B();
b.id(); // outputs B.id but *not* A.id also, as expected

There are other dynamic extension cases that fail, but they may either be (1) homeomorphic with this case or (2) too esoteric to worry about. I will follow up after we decide what to do about the above.

Will attach test file. Please let me know if I messed up.

Changed 10 years ago by sjmiles

Attachment: testDeclareExtend.html added

Test case for dynamic (post-declare) class extension

comment:46 Changed 10 years ago by Eugene Lazutkin

(In [20240]) dojo.declare: minor clean up, renaming, and speeding up, !strict, refs #9862.

comment:47 Changed 10 years ago by Eugene Lazutkin

(In [20241]) dojo.declare: different way to produce empty functions closure-free, !strict, refs #9862.

comment:48 in reply to:  45 Changed 10 years ago by Eugene Lazutkin

Replying to sjmiles:

... but ran into this problem:

Both your examples work for me. Please double-check that you use the recent trunk. If everything is ok, but the problem is there, can you host it somewhere so I can access it, yet you still see the problem? You should have an account at dojotoolkit.org to host the test.

comment:49 Changed 10 years ago by sjmiles

Confirmed this is working against trunk, thank you.

It also seems that dojo.extend will simply work for declared classes now and we don't need any special class extend, correct? In that case I don't have any problem with removing that feature.

comment:50 in reply to:  47 Changed 10 years ago by sjmiles

Replying to elazutkin:

(In [20241]) dojo.declare: different way to produce empty functions closure-free, !strict, refs #9862.

Well done. I'm still not sure if it matters, but now we don't have to worry about it.

comment:51 Changed 10 years ago by Eugene Lazutkin

(In [20276]) dojo.declare: refactoring, size reduction, speed improvements, adding extend() like in the old dojo.declare(), !strict, refs #9862.

comment:52 Changed 10 years ago by Eugene Lazutkin

(In [20277]) dojo.declare: more refactoring to conserve space, speed up, and process edge cases, !strict, refs #9862.

comment:53 Changed 10 years ago by Eugene Lazutkin

(In [20278]) dojo.declare: more test cases with esoteric inheritance, !strict, refs #9862.

comment:54 Changed 10 years ago by Eugene Lazutkin

(In [20279]) dojo.declare: removing extra spaces, !strict, refs #9862.

comment:55 Changed 10 years ago by Eugene Lazutkin

(In [20284]) dojo.declare: speed up and size reduction, !strict, refs #9862.

comment:56 Changed 10 years ago by Eugene Lazutkin

(In [20315]) dojo.declare: more tests, !strict, refs #9862.

comment:57 Changed 10 years ago by Eugene Lazutkin

(In [20525]) dojo.declare: updating the benchmark and the tests for new interface, !strict, refs #9862.

comment:58 Changed 10 years ago by Eugene Lazutkin

Missing commit: [20524].

comment:59 Changed 10 years ago by Eugene Lazutkin

Missing commit: [20527].

comment:60 Changed 10 years ago by bill

(In [20616]) Remove vestigal comment, refs #9862 !strict.

comment:61 Changed 10 years ago by bill

See also #10169 and #10170, which were broken by the [20284] commit to this ticket.

comment:62 Changed 10 years ago by Eugene Lazutkin

(In [20700]) declare: adding immediate parents to the meta information, !strict, refs #9862.

comment:63 Changed 10 years ago by Eugene Lazutkin

Added dojo.declare and dojo.safeMixin to the 1.4 release nodes, updated dojo.declare docs on Dojo Campus (1st draft).

comment:64 Changed 10 years ago by Eugene Lazutkin

Description: modified (diff)

comment:65 Changed 10 years ago by Eugene Lazutkin

(In [20738]) dojo.declare: performance improvements, minor API updates, !strict, refs #9862.

comment:66 Changed 10 years ago by Eugene Lazutkin

(In [20739]) dojo.declare: updating the benchmark, !strict, refs #9862.

comment:67 Changed 10 years ago by bill

[20738] is broken, in dijit/tests/test_TitlePane.html on example 3, when you open the pane the TabContainer is not visible. (Found this from the automated regression tests.) Probably it's another problem with this.inherited() not hitting all the classes but I didn't trace it down.

comment:68 Changed 10 years ago by Eugene Lazutkin

The last change breaks #10300.

comment:69 Changed 10 years ago by Eugene Lazutkin

(In [20755]) dojo.declare: regression cleanup, !strict, fixes #10300, refs #9862.

comment:70 in reply to:  67 Changed 10 years ago by Eugene Lazutkin

Replying to bill:

I think I fixed it now --- all tests are good, and both reported cases work as expected (thx to Doug and you), but please double-check that.

comment:71 Changed 10 years ago by bill

Unfortunately it's still broken (although TitlePane is working). Using dojo.declare() from [20755] on IE8, dijit/tests/layout/ContentPaneLayout.html gets 4 failures. It works when using dojo.declare() from [20700]

comment:72 Changed 10 years ago by Eugene Lazutkin

(In [20791]) dojo.declare: fixing the kinks reported by Bill (thx!), !strict, refs #9862.

comment:73 Changed 10 years ago by Eugene Lazutkin

(In [20796]) dojo.declare: inlining and some code reduction, !strict, refs #9862.

comment:74 Changed 10 years ago by Eugene Lazutkin

(In [20797]) dojo.declare: removing unnecessary assignments, !strict, refs #9862.

comment:75 Changed 10 years ago by Eugene Lazutkin

(In [20798]) dojo.declare: cleanup, !strict, refs #9862.

comment:76 Changed 10 years ago by Eugene Lazutkin

(In [20799]) dojo.declare: making "constructor" a named variable, reformulating inherited() to gain back performance on Chrome, !strict, refs #9862.

comment:77 Changed 10 years ago by Eugene Lazutkin

(In [20800]) dojo.declare: eliminating trailing spaces, !strict, refs #9862.

comment:78 Changed 10 years ago by Eugene Lazutkin

(In [20801]) dojo.declare: minor tweaks to tests and benchmarks, !strict, refs #9862.

comment:79 Changed 10 years ago by Eugene Lazutkin

Amazingly adding a single dictionary lookup made inherited() on Chrome/V8 6 (six) times slower:

var op = Object.prototype;
// ...
var opf = op[name]; // THIS ONE! NOT IN THE LOOP!

I can't believe it is so slow. If you look at inherited() it is a relatively large function with dozens operations. Yet, opf overweighted them all. I suspect that Object.prototype is implemented differently than normal JavaScript objects, because all other lookups are very fast.

comment:80 Changed 10 years ago by bill

These recent changes break dijit again. The button to open a menu for a CombButton (see Save button in test_Button.html) doesn't open the menu. It works in [20791] but fails in [20800].

comment:81 Changed 10 years ago by Eugene Lazutkin

(In [20810]) dojo.declare: fixing another gotcha by Bill (thx!), !strict, refs #9862.

comment:82 Changed 10 years ago by Eugene Lazutkin

Resolution: fixed
Status: assignedclosed

All found problems and enhancements should go in separate tickets.

comment:83 Changed 10 years ago by Eugene Lazutkin

(In [20880]) dojox.lang.oo: removing unneeded files, which were promoted to dojo.declare, !strict, refs #9862.

Note: See TracTickets for help on using tickets.