#10788 closed defect (fixed)
[patch][cla]strange inheritance behavior
Reported by: | ben hockey | Owned by: | Eugene Lazutkin |
---|---|---|---|
Priority: | high | Milestone: | 1.5 |
Component: | Core | Version: | 1.4.0 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
in the test file i've attached, i'm overriding the buildRendering in dijit._Widget and dijit._Templated prototypes. if you switch between 1.3 and 1.4 you'll see some different behavior.
dojo 1.3
- you'll see the console.log showing that the new buildRendering is being used
dojo 1.4
- the new buildRendering is not used unless i give my.foo a buildRendering method which calls this.inherited
i've marked this as critical because it's not quite a blocker since there's a workaround.
i'd also be happy to be told that there's something wrong with my example.
Attachments (6)
Change History (28)
Changed 11 years ago by
Attachment: | proto.html added |
---|
comment:1 Changed 11 years ago by
Owner: | changed from anonymous to Eugene Lazutkin |
---|---|
Status: | new → assigned |
comment:2 Changed 11 years ago by
i've updated my test file but still have the same issues. declaring a buildRendering in my.foo and calling this.inherited is still a workaround.
comment:3 Changed 11 years ago by
i'm not sure if this patch is exactly how you would like to solve the issue but it works and should hopefully at least point to where the issue is coming from - see declare.diff attached.
Changed 11 years ago by
Attachment: | declare.diff added |
---|
patch for declare to update meta data in constructor
comment:4 Changed 11 years ago by
oh... and do you think this bug would be appropriate for inclusion in the 1.4.2 release? i feel like it is quite important.
comment:5 Changed 11 years ago by
Milestone: | tbd → 1.4.2 |
---|---|
severity: | critical → normal |
comment:6 Changed 11 years ago by
The patch appears to be correct in general, but I need to see if it doesn't violate anything else. For example, right off the bat: _meta
should be used only if it is defined (it is not always the case).
comment:7 Changed 11 years ago by
Summary: | strange inheritance behavior → [patch][cla]strange inheritance behavior |
---|
comment:8 Changed 11 years ago by
elazutkin: This ticket has milestone set for 1.4.2, is this something that should go in the 1.4.2 release? Adam just cut an RC for 1.4.2, but if we should wait for this ticket, that would be good to know, so we can adjust the 1.4.2 release according to when it is fixed.
comment:9 Changed 11 years ago by
I should have noted that I had a chat with Eugene about this yesterday and he didn't think it would make the 1.4.2 release. We should make that decision final by changing the milestone... going once, twice?
comment:10 Changed 11 years ago by
Oh, OK. Looks like there was also a possibly related issue mentioned in the dojo-interest list today: http://mail.dojotoolkit.org/pipermail/dojo-interest/2010-March/043804.html
although it is light on reproducible details.
As this seems like a regression from 1.3, it would be good to address if not in a 1.4.2 release, maybe a 1.4.3.
If we need to take another week to get this one sorted out for a 1.4.2 release, would that be OK (depending on what Eugene says)? AFAIK, there is nothing pressing the 1.4.2 release except for the xss fixes for test files, but that can wait another week if that helps resolve the issue.
comment:11 Changed 11 years ago by
behavior has definitely changed since 1.3 and can be seen in the attached test case. even using the preferred Class.extend rather than changing the prototype directly still does not give the expected behavior. the attached files show use cases, which i assume would be typical, where changes to a superclass will not be inherited by subclasses (unless the workaround shown in the test case is applied).
comment:12 Changed 11 years ago by
Ben forgot to add a link to his test case: http://neonstalwart.dojotoolkit.org/test/declare.html
Again it is not clear if his patch fixes it.
comment:13 follow-up: 14 Changed 11 years ago by
One more note: dojo.declare()
uses the mixin model to emulate the multiple inheritance. It means that only one class can be a true parent (in the single inheritance sense), the rest are going to be copied. If anybody changes a class, which was already copied, the change would not be reflected in copies for obvious reasons. It was true for the original dojo.declare()
, it is true for the current implementation.
comment:14 Changed 11 years ago by
Replying to elazutkin:
One more note:
dojo.declare()
uses the mixin model to emulate the multiple inheritance. It means that only one class can be a true parent (in the single inheritance sense), the rest are going to be copied. If anybody changes a class, which was already copied, the change would not be reflected in copies for obvious reasons. It was true for the originaldojo.declare()
, it is true for the current implementation.
the test case at http://neonstalwart.dojotoolkit.org/test/declare.html demonstrates that my "patch" is not sufficient. please just consider the patch to be only indicative - it is not truly a complete patch. these 2 links are similar to the first proto.html file attached to this ticket - http://neonstalwart.dojotoolkit.org/test/proto1.3.html uses dojo 1.3 and http://neonstalwart.dojotoolkit.org/test/proto1.4.html uses dojo 1.4
comment:15 Changed 11 years ago by
I looked at declare.html
example and it should not work because only A
is inherited, and B
and C
are copied. Obviously it doesn't work with Dojo 1.3 either.
Having said that the _Widget
/_Templated
example should work => there is a bug, which should be fixed.
comment:16 follow-up: 19 Changed 11 years ago by
i have a couple more test files to attach - these ones show a difference in behavior when you call this.inherited with the name of the function you want to call
dojo.declare('my.Base', null, { foo: function(){ console.log('Base::foo'); } }); dojo.declare('my.Thing', my.Base, { foo: function(){ console.log('Thing::foo'); }, bar: function(){ console.log('Thing::bar'); this.inherited('foo', arguments); } });
i'll attach the files to the ticket but here are links to live copies of these files http://neonstalwart.dojotoolkit.org/test/inherit1.4.html http://neonstalwart.dojotoolkit.org/test/inherit1.3.html
Changed 11 years ago by
Attachment: | inherit1.3.html added |
---|
Changed 11 years ago by
Attachment: | inherit1.4.html added |
---|
comment:17 Changed 11 years ago by
Milestone: | 1.4.2 → 1.5 |
---|
comment:19 Changed 11 years ago by
Replying to neonstalwart:
i have a couple more test files to attach - these ones show a difference in behavior when you call this.inherited with the name of the function you want to call
This one is explainable: the method name overrides the annotated name. It should not be used to call a superclass' method with a different name.
The algorithm of "inherited" works like that:
- arguments.caller gives us a method, which was used to call "inherited". We need to find it in our hierarchy in order to find next method to call.
- The "right" way to do it is to inspect all properties of a prototype in order to find the match. The process should be repeated for all objects in the prototype chain until the method is found. If it was not found, the other possibility is that it was dynamically attached to the instance => we need to inspect all properties of the instance checking for our method. If it was not found there, we give up.
- Obviously the previous algorithm while exhausting is very too slow:
for...in
is very slow, and the amount of checks is too big.
- So we use a shortcut --- we need to know the method's name in order to eliminate
for...in
loops. Now our search is linear. It is proportional to the length of the inheritance chain. Using caching we can eliminate loops in most cases.
- The source of method's name is a property on the method itself called "nom". If the method was added manually, it is not decorated, so the name attribute of "inherited" is used. It overrides "nom" to cover another case: then already decorated method is added to an object with a different name.
- So the name override is used for dynamic methods to indicate their name to "inherited" rather than who we call next.
Is it different from 1.3? I think it is different as your examples demonstrate. Does it warrant the change? Only if it doesn't increase the code and doesn't break anything.
Changed 11 years ago by
Attachment: | ben_declare.html added |
---|
Minimal example that demonstrates the bug found by Ben.
comment:20 Changed 11 years ago by
I corrected the bug and explained the behavior in http://docs.dojocampus.org/dojo/declare
comment:21 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Methods are decorated by
dojo.declare()
, yet you use raw functions and construct prototypes (as far asbuildRendering()
is concerned) manually. Here you go.Most probably you should use
Class.extend()
to replace those methods. I'll post a snippet later.