#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 )
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)
Change History (85)
comment:1 Changed 11 years ago by
Status: | new → assigned |
---|
comment:2 Changed 11 years ago by
comment:3 Changed 11 years ago by
comment:4 Changed 11 years ago by
comment:5 Changed 11 years ago by
comment:6 Changed 11 years ago by
comment:7 follow-up: 8 Changed 11 years ago by
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 Changed 11 years ago by
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:
- 9/15 http://archive.dojotoolkit.org/dojo-2009-09-15/dojotoolkit/dojo/_base/
- 9/16 http://archive.dojotoolkit.org/dojo-2009-09-16/dojotoolkit/dojo/_base/
- 9/17 http://archive.dojotoolkit.org/dojo-2009-09-15/dojotoolkit/dojo/_base/
They all use old dojo.declare (you can inspect files).
These are links to the test in question:
- 9/15 http://archive.dojotoolkit.org/dojo-2009-09-15/dojotoolkit/dijit/tests/layout/ContentPaneLayout.html
- 9/16 http://archive.dojotoolkit.org/dojo-2009-09-16/dojotoolkit/dijit/tests/layout/ContentPaneLayout.html
- 9/17 http://archive.dojotoolkit.org/dojo-2009-09-17/dojotoolkit/dijit/tests/layout/ContentPaneLayout.html
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 11 years ago by
Attachment: | declare-old.js added |
---|
the old version of dojo.declare, can be used for testing and as a reference
comment:9 follow-up: 13 Changed 11 years ago by
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:10 Changed 11 years ago by
comment:11 Changed 11 years ago by
comment:12 Changed 11 years ago by
comment:13 Changed 11 years ago by
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:15 Changed 11 years ago by
comment:16 Changed 11 years ago by
[20156] emulates Bill's case and it works now. Still we need to test the real thing.
comment:17 Changed 11 years ago by
comment:19 Changed 11 years ago by
comment:20 Changed 11 years ago by
comment:22 Changed 11 years ago by
comment:25 follow-up: 27 Changed 11 years ago by
Could you check dijit/tests/_Templated-widgetsInTemplate.html? I'm getting a hang in the c3mro code. Thanks.
comment:26 Changed 11 years ago by
comment:27 Changed 11 years ago by
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 11 years ago by
Ah, ok thanks! I'll fix that error in _Templated-widgetsInTemplate.html (it wasn't intended to include _Widget twice).
comment:29 follow-up: 30 Changed 11 years ago by
(In [20173]) TestLayoutWidget? (unintentionally) extended from _Widget twice. Refs #9862.
comment:30 Changed 11 years ago by
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 11 years ago by
comment:32 Changed 11 years ago by
comment:33 follow-up: 36 Changed 11 years ago by
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 follow-up: 37 Changed 11 years ago by
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 11 years ago by
correction: the resize() call in the above example doesn't give an error - it just doesn't call the inherited function.
comment:36 follow-up: 38 Changed 11 years ago by
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 follow-up: 39 Changed 11 years ago by
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 Changed 11 years ago by
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 ofnull
and[A]
instead ofA
). Frankly I don't see any compelling reasons for "double-mixin".
OK - I'm fine with that as a solution.
comment:39 follow-up: 40 Changed 11 years ago by
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 follow-up: 41 Changed 11 years ago by
Replying to toonetown:
I can write up a test case, if you like.
The current code handles two dynamic things:
- One of the prototypes is modified dynamically, and the new method calls
this.inherited()
directly. Obviously it needs to supply a name. - 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 Changed 11 years ago by
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 11 years ago by
comment:43 Changed 11 years ago by
comment:44 Changed 11 years ago by
comment:45 follow-up: 48 Changed 11 years ago by
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 11 years ago by
Attachment: | testDeclareExtend.html added |
---|
Test case for dynamic (post-declare) class extension
comment:46 Changed 11 years ago by
comment:47 follow-up: 50 Changed 11 years ago by
comment:48 Changed 11 years ago by
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 11 years ago by
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 Changed 11 years ago by
comment:51 Changed 11 years ago by
comment:52 Changed 11 years ago by
comment:53 Changed 11 years ago by
comment:54 Changed 11 years ago by
comment:55 Changed 11 years ago by
comment:57 Changed 11 years ago by
comment:61 Changed 11 years ago by
comment:62 Changed 11 years ago by
comment:63 Changed 11 years ago by
Added dojo.declare
and dojo.safeMixin
to the 1.4 release nodes, updated dojo.declare
docs on Dojo Campus (1st draft).
comment:64 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:65 Changed 11 years ago by
comment:66 Changed 11 years ago by
comment:67 follow-up: 70 Changed 11 years ago by
[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:69 Changed 11 years ago by
comment:70 Changed 11 years ago by
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 11 years ago by
comment:72 Changed 11 years ago by
comment:73 Changed 11 years ago by
comment:74 Changed 11 years ago by
comment:76 Changed 11 years ago by
comment:77 Changed 11 years ago by
comment:78 Changed 11 years ago by
comment:79 Changed 11 years ago by
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 11 years ago by
comment:81 Changed 11 years ago by
comment:82 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
All found problems and enhancements should go in separate tickets.
(In [20068]) lang.oo: simplifications and performance improvements across the board, !strict, refs #9862.