make extend() on a dojo.declare'd class create new class rather than modifying original

[Kris Zyp]

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).

[bill]

Hmm, I wouldn't call that a design defect, you can easily get a new extended class by:

NewDialog = dojo.declare(dijit.Dialog, {foo:function(){console.log('hi')}});

Note that dojo.extend(dijit.Dialog, {...}) also modifies Dialog, do you also consider that a design defect?

[Kris Zyp]

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.

[bill]

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?

[Eugene Lazutkin]

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.

[Kris Zyp]

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.

[bill]

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.)

[Eugene Lazutkin]

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. ;-)

[bill]

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)

[Kris Zyp]

Yes, I would have hoped it would support multiple inheritance (and Compose's extend does).

[ben hockey]

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.

[Eugene Lazutkin]

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?

[ben hockey]

same as mentioned in this thread ​http://thread.gmane.org/gmane.comp.web.dojo.devel/13527/

[bill]

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.

[dylan]

We should just close the ticket as createSubclass() solves the issue. Dojo 2 takes a different approach anyway.