Opened 8 years ago

Closed 3 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:1 Changed 8 years ago by 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?

Last edited 8 years ago by bill (previous) (diff)

comment:2 Changed 8 years ago by 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.

comment:3 Changed 8 years ago by 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?

comment:4 in reply to:  2 Changed 8 years ago by Eugene Lazutkin

Resolution: invalid
Status: newclosed

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.

Last edited 8 years ago by Eugene Lazutkin (previous) (diff)

comment:5 Changed 8 years ago by Kris Zyp

Resolution: invalid
Status: closedreopened
Type: defectenhancement

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 Changed 8 years ago by bill

Summary: extend() on a dojo.declare'd class modifies originalmake 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 in reply to:  6 Changed 8 years ago by 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. ;-)

comment:8 Changed 8 years ago by 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)

comment:9 Changed 8 years ago by Kris Zyp

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

comment:10 Changed 8 years ago by 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.

comment:11 in reply to:  10 Changed 8 years ago by 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?

comment:12 Changed 8 years ago by ben hockey

comment:13 Changed 7 years ago by bill

Owner: set to Eugene Lazutkin
Priority: highlow
Status: reopenedassigned

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 3 years ago by dylan

Milestone: 2.01.11
Resolution: fixed
Status: assignedclosed

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

Note: See TracTickets for help on using tickets.