Opened 11 years ago
Closed 7 years ago
#13600 closed enhancement (fixed)
make extend() on a dojo.declare'd class create new class rather than modifying original
Reported by: | Kris Zyp | Owned by: | Eugene Lazutkin |
---|---|---|---|
Priority: | low | Milestone: | 1.11 |
Component: | Core | Version: | 1.6.1 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
Calling extend() on a class should return a new extended class without modifying the original class. extend() does return an extended class, but it modifies the original class to do so (the original class is in fact what is returned):
> var d = dijit.Dialog(); d.foo -> undefined > NewDialog = dijit.Dialog.extend({foo:function(){console.log('hi')}}); > nd = new NewDialog(); > nd.foo(); // looks like extend() properly created my new class with foo method hi > d.foo() // ugh, the original class was modified hi
Of course this was by design, I am filing this as a design defect. We should be able to extend a class from the class without icky side-effects. I am not how much extend() is used, but we probably can't change this in 1.x, but it should definitely be fixed in 2.0 (replacing dojo.declare with compose would fix it).
Change History (14)
comment:2 follow-up: 4 Changed 11 years ago by
Ah, I see, as long as you don't use .extend() it works just fine ;).
dojo.extend doesn't really carry the expectation of dojo.declare. It a standalone function. It is merely poorly named, it is not coupled with another function. The dojo.declare's extend should match dojo.declare's behavior. Side-effect based programming is just plain sloppy and dangerous, we should be encouraging functional over imperative.
comment:3 Changed 11 years ago by
Oh, using extend() when it makes sense works fine, for example in all the NodeList-*.js files that add methods to the NodeList class.
If you are really suggesting to make clz.extend(hash)
do the same thing as declare(clz, hash)
then I'd say, why have two APIs to do the same thing?
comment:4 Changed 11 years ago by
Resolution: | → invalid |
---|---|
Status: | new → closed |
Replying to kzyp:
Calling extend() on a class should return a new extended class without modifying the original class.
Who said so?
This function is grandfathered, and was added when people started to complain about "new declare broke our code!!!". You can look at all sordid details in the commit history --- I tried to document accurately all changes and fixes.
And, like Bill said, use declare()
to define new class:
var NewClass = dojo.declare([OldClass, NewMixin], {});
Or did you want to propose some cool enhancement, which I missed somehow? If this is the case, please open an enhancement ticket with a detailed proposal.
comment:5 Changed 11 years ago by
Resolution: | invalid |
---|---|
Status: | closed → reopened |
Type: | defect → enhancement |
The purpose is so that you can extend directly from a reference to a class without having to obtain a reference to dojo.declare (for example you could extend a class without needing to load the declare module in case the class was constructing from alternate means). Anyway, I changed this to an enhancement, per your request.
I am certainly not denying the grandfathered nature of extend, that is why I marked it 2.0.
comment:6 follow-up: 7 Changed 11 years ago by
Summary: | extend() on a dojo.declare'd class modifies original → make extend() on a dojo.declare'd class create new class rather than modifying original |
---|
I can see the advantage if you don't have to put "dojo/_base/declare" in your dependency vector.
But superclass.newExtend(hash) seems limited though since you can only specify one superclass (ie, you can't specify multiple inheritance), so not sure how useful it would be in practice. (I did check the documentation for Compose, but the multiple-inheritance example calls the Compose() method directly.)
comment:7 Changed 11 years ago by
Replying to bill:
... so not sure how useful it would be in practice.
In practice it is used in users projects. I am not debating merits of the existing implementation, or Kris' proposal. I just don't see how we can change its semantics (or rename it, or remove it) now.
Coming back to the original implementation: I think it was based on dojo.extend()
, which modifies its first argument object, doesn't return new object, and takes exactly one mixin at a time. In fact it is dojo.extend()
disguised as a method + it adds some small metadata required for inherited()
machinery to work.
When I was redoing dojo.declare()
I removed extend()
method feeling that it is redundant. But pretty soon I found myself adding it and other "legacy" stuff back. :-( Hopefully we can simplify things in 2.0. One thing we can do is to remove extend()
completely. ;-)
comment:8 Changed 11 years ago by
To be clear, I meant that I wasn't sure how useful Kris' suggested extend() would be, since it apparently didn't have support for multiple inheritance. But maybe it could support multiple inheritance, ex:
var newClass = superClass.extend([mixin1, mixin2], hash);
(with the first argument optional)
comment:9 Changed 11 years ago by
Yes, I would have hoped it would support multiple inheritance (and Compose's extend does).
comment:10 follow-up: 11 Changed 11 years ago by
we'll probably end up having a more involved discussion on the mailing list at some point but since it was mentioned on this ticket, i just wanted to register my support for exploring the possibility of using compose in 2.0.
comment:11 Changed 11 years ago by
Replying to neonstalwart:
we'll probably end up having a more involved discussion on the mailing list at some point but since it was mentioned on this ticket, i just wanted to register my support for exploring the possibility of using compose in 2.0.
URL of Compose?
comment:12 Changed 11 years ago by
same as mentioned in this thread http://thread.gmane.org/gmane.comp.web.dojo.devel/13527/
comment:13 Changed 10 years ago by
Owner: | set to Eugene Lazutkin |
---|---|
Priority: | high → low |
Status: | reopened → assigned |
This functionality was added in [27667], but called createSubclass(), so the only thing left IIUC would be to rename createSubclass() to extend(), and then rename the old extend() to something else. Or we could just close this ticket.
comment:14 Changed 7 years ago by
Milestone: | 2.0 → 1.11 |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
We should just close the ticket as createSubclass() solves the issue. Dojo 2 takes a different approach anyway.
Hmm, I wouldn't call that a design defect, you can easily get a new extended class by:
Note that dojo.extend(dijit.Dialog, {...}) also modifies Dialog, do you also consider that a design defect?