Opened 10 years ago

Closed 8 years ago

#9143 closed defect (wontfix)

dojo.hitch and this.inherited don't play well together.

Reported by: dante Owned by: sjmiles
Priority: high Milestone: 2.0
Component: Core Version: 1.3.0
Keywords: Cc: Eugene Lazutkin
Blocked By: Blocking:

Description

They do in most cases, though inherited fails when being called as a hitched function. eg:

method: function(){
   dojo.fadeOut({ node:this.domNode,
       onEnd: dojo.hitch(this, "inherited", arguments)
   }).play();
},

Prior to 1.4 (trunk), the above example worked, as nothing was being passed to the hitched function, thus nothing being passed to the inherited() call in the [undocumented] newArgs position. My FX changes in 1.4 make onEnd fire being passed the node for the animation. The node being passed to inherited()'s newArgs position is causing the value of 'a' to a value, and not an array. a is later passed in f.apply(this, a)

Technically, my FX changes are back-compat-breaking ... but I'm of the opinion inherited() should be able to handle additional passed arguments safely. Anything attempting to pass additional arguments to a hitched() inherited function will break. Half of animation events already pass values, breaking the use of hitched inherited in any of those callbacks, though my changes added values being passed to onEnd and beforeBeing, thus breaking hitched inherited usage. (Turns out I am using onEnd: dojo.hitch(this, "inherited", arguments) in a couple of widgets, and suspect others are too)

something needs fixed before 1.4, either documenting the shortcoming of inherited, backing out the FX onEnd/beforeBegin change, make inherited convert all additional arguments into an array in the event a non-array newArg is encountered ... It seems the latter can be done while retaining back compat at a small cost, but I am unclear on the expected use of inherited() args as they are entirely undocumented.

Attachments (2)

inherited-fail.html (1.2 KB) - added by dante 10 years ago.
shows a breaking hitched() inherited call
declare.patch (4.9 KB) - added by dante 10 years ago.

Download all attachments as: .zip

Change History (15)

Changed 10 years ago by dante

Attachment: inherited-fail.html added

shows a breaking hitched() inherited call

comment:1 Changed 10 years ago by Eugene Lazutkin

You may need to add a name argument as well --- this.inherited() relies on the call stack to infer its name and on the validity of the 1st/2nd parameter, which should be a valid active arguments.

Do something like this:

method: function(){
   dojo.fadeOut({ node:this.domNode,
       onEnd: dojo.hitch(this, "inherited", "method", arguments)
   }).play();
},

These are valid signatures of inherited:

// In all cases args should be a valid arguments pseudo variable.
// It is used to refer to the original caller function,
// which was called with these arguments.

// the full form
this.inherited(name, args, newArgs);

// name = args.callee.nom, newArgs = args
this.inherited(args);

// name = args.callee.nom
this.inherited(args, newArgs);

// newArgs = args
this.inherited(name, args);

All these "take from arguments what we want" business is brittle, most probably you are better off specifying everything manually. But it still use args to get callee, so ultimately you may need to specify something like this:

method: function(){
   dojo.fadeOut({ node:this.domNode,
       onEnd: dojo.hitch(this, "inherited", "method", {callee: arguments.callee})
   }).play();
},

Hmm, I should check if dojox.lang.oo.declare is bug-compatible in this respect. OTOH I analyzed our codebase and we don't have code like in your example anywhere. All questionable cases were rewritten to avoid using this.inherited().

comment:2 Changed 10 years ago by dante

I call inherited this way in RadioGroup? (at the very least), so not sure how thoroughly you checked. l:139 dojox/layout/RadioGroup.js

			onEnd: dojo.hitch(this,"inherited", arguments)

it is what led me to investigate this in the first place (when attached to an FX event handler, the node being passed causes the newArgs to be that node, thinking mistakenly a new set of arguments wants to be passed.

Attached is a patch which seems to fix this issue, and is technically an enhancement to inherited() supported syntax (any positional non-array arguments after the args object are appended to the calling arguments)

Changed 10 years ago by dante

Attachment: declare.patch added

comment:3 Changed 10 years ago by Eugene Lazutkin

I didn't check !DojoX thoroughly --- concentrated on Core and Dijit mostly. Now I have a pattern to look for.

comment:4 Changed 10 years ago by dante

Owner: changed from anonymous to sjmiles

scott, do you have any thoughts here as well? the arg shifting stuff was difficult to workaround, but I think I retained complete back compat, and also allowing for hitched functions to appened additional arguments. I have some tests, my fx unit tests (that are currently failing) now work, and the attached works. Can expand unit tests after review.

comment:5 Changed 10 years ago by Eugene Lazutkin

I see the problem --- you still want to use the old arguments ignoring any additional garbage, which is generated by event callbacks. You can easily do it like that:

method: function(){
   var a = arguments;
   dojo.fadeOut({ node:this.domNode,
       onEnd: dojo.hitch(this, function(){ this.inherited(a); })
   }).play();
},

The price is obvious: one extra wrapper. If you do want to append extra arguments (like in the patch), it'll be a little bit more involved, but still easily doable.

The patch looks like a special casing --- I think that 99% of valid use cases do not need this, but I can be convinced that it is the valid way to go. ;-)

Another related thought: we can expose _findMethod() with friendlier API, which finds the inherited method, and call it ourselves. In this case we may gain some speed and free to mangle our arguments as we wish. Possible API is:

findInherited: function(/* String? */ name, args)

so you can do:

var f = this.findInherited(arguments);
// now use it
f.apply(this, arguments);
// call it again with arbitrary arguments
f.call(this, 1, 2, 3);

comment:6 Changed 10 years ago by bill

I don't think we should change/enhance inherited() to handle this. It's unfortunate that there's no API doc for the inherited() method but it's clear from the code that the API is:

inherited(/*String?*/ methodName, arguments, /*Object[]?*/ newArgArray)

(where arguments is literally the arguments variable built-in to javascript).

The calling code is effectively doing this (thanks to dojo.hitch() creating a partial:

this.inherited(arguments, node);

Since node isn't an array, the calling code is breaking the API.

We could enhance inherited() to handle the above case as though the newArgs == [node], but then we will run into ambiguity with a call like:

this.inherited(arguments, [1,2,3]);

It's unclear if that means to call superclass.foo(1, 2, 3) or superclass.foo( [1,2,3] ).

comment:7 Changed 10 years ago by dante

to be the passing of an array explicitly is weird (just as publish requiring an array is weird). My patch continues to support this, but creates an array in the case where a partial arg was appended. You are still able to override the args as you were.

Frankly I don't care what happens, provided we merge at least the documentation part of the patch, and the style cleanups. declare.js is hideous to read, and 'this.inherited' is entirely undocumented in Dojo, which is unacceptable.

comment:8 Changed 10 years ago by dante

(In [17307]) refs #9143 - doc and style updates for declare.js - no code changes, just part of the patch. \!strict

comment:9 Changed 10 years ago by bill

Yes, thanks for adding API doc for inherit().

As you mentioned the array discussion is the same as #8738, and we shouldn't change it in 1.0 for the same reason: because it's error-prone when your new argument is array-like. For example, if developers can do this:

this.inherited(arguments, 5);

then they will expect that this works similarly:

this.inherited(arguments, myNodeList);

(but it wouldn't, for backwards-compatibility reasons; it would rip apart myNodeList into separate arguments)

comment:10 Changed 10 years ago by dante

(In [17312]) refs #9143 - adding unit tests for fx showing inherited in a callback (two ways)

comment:11 Changed 10 years ago by dante

Milestone: 1.42.0

so can we change this ticket to "drop 'name' from inherited, and support partial arguments" and target it 2.0? Or do we have a roadmap for notes on behavioral changes we want to normalize (we should make our functions behave consistently, and I like hitch()'s pattern of n-args rather than explicit array) So I suggest we drop using an array anywhere we are.

comment:12 Changed 10 years ago by Eugene Lazutkin

Cc: Eugene Lazutkin added

comment:13 Changed 8 years ago by bill

Resolution: wontfix
Status: newclosed

Since we will probably get rid of this.inherited() in 2.0, closing this as wontfix.

Note: See TracTickets for help on using tickets.