Opened 9 years ago

Closed 3 years ago

#11738 closed defect (patchwelcome)

dojo.Stateful calls watch callbacks if value did not change

Reported by: John Hann Owned by: Kris Zyp
Priority: low Milestone: 1.13
Component: Core Version: 1.5
Keywords: Cc:
Blocked By: Blocking:

Description

The set() method in dojo.Stateful calls the watch callbacks even if the value passed into set() has not changed. This puts the onus on the callback handlers to check. When doing repetitive operations, this also wastes unnecessary cpu cycles.

Change History (20)

comment:1 Changed 9 years ago by bill

Component: GeneralCore
Owner: changed from anonymous to Kris Zyp

I noticed this too and worked around it in _Widget, but it would be better to fix it in Stateful itself.

comment:2 Changed 9 years ago by ben hockey

for the sake of argument... what if i consider it significant that a property was set even if it was the same value? is it appropriate that Stateful decides that a set with the same value should be insignificant?

an example of this is where i might be monitoring a property of a Stateful object and trying to record an average of it's values. in this case, i care about values which have been set even if the value is the same. for example, the average of 8, 7, 2 is very different to the average of 8, 8, 8, 8, 7, 7, 2.

my example is contrived and i'm not personally doing anything like this but just wanted to make sure this kind of argument is considered... i'll be easily persuaded to let it go as long as the argument is considered.

comment:3 Changed 9 years ago by John Hann

@neonstalwart Answer:

dojo.connect(myStatefulObject, 'set', reCalcAverageFunc);

:)

But seriously, that's a good point -- and one I was hoping would be mentioned. There could be use cases in which you want to detect calls to set, not just changes in state. I think the answer is something like the code above. If you're subclassing, then the solution is even more straightforward: override the set method.

Unless I am wrong, the purpose of Stateful's get/set/watch is to detect state changes. Right?

I filed this bug because I am trying to track down the source(s) of state change notifications being called multiple times in my code. This wasn't the primary cause of the duplications, but it definitely contributed to the overall flood of notifications. :(

comment:4 Changed 9 years ago by Kris Zyp

We have been trying to follow the Mozilla/SpiderMonkey?'s API on this one: o = {foo:3} o.watch("foo",console.log) o.foo=3; outputs: foo 3 3 (The watch callback is indeed fired even when the value is the same)

Is it worth deviating here?

comment:5 Changed 9 years ago by John Hann

@kzyp: Imho, the answer to that is the same as the answer to this question:

Is the purpose of dojo.Stateful to emulate the moz API or to capture state changes?

comment:6 Changed 9 years ago by ben hockey

@kzyp i think the existing api is reasonable - it leaves it to the "watcher" to decide the significance of the value being set.

@unscriptable agreed... the question is: "are we watching changes or are we watching calls to set?"

to continue my argument, we are watching calls to set but i might also argue that semantically a call to set is a change of state even if the value being set is the same - something interacted with the object and in some circumstances, that may count as enough to be considered a change of state.

the watch callback is passed the old and the new value so that the watcher can decide about the significance of the value that was set. from this, you can produce an onFooChange that you can connect to if you're interested only in events that change values of the foo property. none of this saves an cpu cycles but i think that's probably not the most significant part of unscriptable's concern - that might just be bonus points if it's possible to achieve. :)

comment:7 Changed 9 years ago by John Hann

@neonstalwart I respectfully disagree. A state change means something changed. "interaction" != "state change". If you're detecting interactions, method replacement (e.g. dojo.connect on the setter) is a better possible implementation.

Fwiw, I could obviously live with the code the way it is. Outside of some extra debugging time (trying to figure out which callbacks were legit and which weren't) this is obviously not a critical issue.

However, I think it's confusing and counter-intuitive to the purpose of the Stateful class. This class was created specifically to track property changes, wasn't it? From the dojo.Stateful source: "Base class for objects that provide named properties with optional getter/setter control and the ability to watch for property changes"

And from the watch method (excerpt): "The function to execute when the property changes. This will be called after the property has been changed. The callback will be called with the |this| set to the instance, the first argument as the name of the property, the second argument as the old value and the third argument as the new value."

Side note: The primary use case for mozilla's watch is for debugging. Interesting discussion at https://bugzilla.mozilla.org/show_bug.cgi?id=394964. (Also, this ticket illustrates some good reasons not to necessarily follow the mozilla spec exactly.)

comment:8 in reply to:  7 ; Changed 9 years ago by ben hockey

Side note: The primary use case for mozilla's watch is for debugging. Interesting discussion at https://bugzilla.mozilla.org/show_bug.cgi?id=394964. (Also, this ticket illustrates some good reasons not to necessarily follow the mozilla spec exactly.)

i would agree that we don't want to add properties to an object when you try to watch a property that doesn't exist - so you're right, we don't want to follow it exactly.

wrt the main point of this discussion (which is whether or not watch should be called when the value is set the same as the existing value) i would think that there should also be a bug (or some other record) discussing our main point. i tried to find such a bug/discussion but couldn't find one. i was hoping we could draw from whatever previous discussion existed and see if there was a convincing reason that we should follow (or not follow) the api.

as i said in my first comment, i'll be easily persuaded to let it go. i'm satisfied that the argument has been heard and considered and so i'll try not to add any more comments unless there is a question specifically for me. i don't know if we have a consensus yet about what to do but to allow this to go forward, i won't oppose whatever decision is made.

comment:9 in reply to:  8 Changed 9 years ago by ben hockey

i would think that there should also be a bug (or some other record) discussing our main point.

to clarify - i mean that i would think that mozilla would have a bug or some record of their discussion about the behavior of watch but i was unable to find one.

comment:10 Changed 9 years ago by John Hann

@neonstalwart: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/watch

Is that what you're looking for - or something deeper like what was going through Brendan Eich's brain in 1997?

comment:11 Changed 9 years ago by John Hann

Related:

The following two valid dojo.Stateful calls don't work the same in mozilla's watch as in dojo.Stateful's:

object.watch(callback); throws a TypeError?: missing argument 1...

object.watch('*', callback); adds a '*' property to object and doesn't work as a wildcard, either

This is important if you don't know for sure if you've got a dojo.Stateful object (or dijit._Widget or other implementor of watch).

Therefore, I've had to sniff for the native watch in my code. This is straightforward: just test for object.watch == Object.prototype.watch.

This should be documented somewhere, imho. I'd do it, but I don't have the rights. :(

comment:12 in reply to:  10 Changed 9 years ago by ben hockey

@neonstalwart: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/watch

Is that what you're looking for - or something deeper like what was going through Brendan Eich's brain in 1997?

i was meaning something more than just the api - eg. reasons for why watch works the way it does; if it was intentional to call watch when the value is set to the same as the existing value. maybe there was some debate about it but then again maybe it was just arbitrary. but please don't spend time looking for it for my sake, i'll be ok with whatever color we paint the bikeshed - i just wanted to make sure we considered a range of colors ;)

comment:13 Changed 9 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

Fixed by commit 22883 (why didn't it show up here?)

comment:14 Changed 9 years ago by bill

Milestone: tbd1.6
Resolution: fixed
Status: closedreopened

Slight problem: [22883] will still report a change when both the old and new value are NaN, which happens with widgets a lot (NumberTextBox, DateTextBox, etc.) Old and new undefined seems to work fine.

comment:15 in reply to:  14 ; Changed 9 years ago by ben hockey

Replying to bill:

Slight problem: [22883] will still report a change when both the old and new value are NaN, which happens with widgets a lot (NumberTextBox, DateTextBox, etc.) Old and new undefined seems to work fine.

and this got me thinking again... the way that Stateful checks for equality needs to be general enough that it's widely applicable. i promise this is not a desperate attempt to be more persistent ;) after giving this some thought i'm not sure that the following example is such an edge case that it should be ignored. i came to this decision by wondering how many questions with examples like this would i think will be posted to the mailing list.

var state = new dojo.Stateful();
state.watch('d', function (prop, old, value) {
    console.log('new value for d: ', value);
});
var date = new Date();
state.set('d', date);    // fires watch

date.setDate(12);
state.set('d', date);    // doesn't fire watch

of course, this same problem would also apply to arrays and objects as values. there are different design decisions a developer could make which might help alleviate this problem (the parent object could try to watch all properties of it's children - as long as they were Stateful) but those add more complexity and wouldn't work for Dates (i'm open to hearing about designs that take all of this into account). i'm concerned that having Stateful decide what counts as equality limits a developer's options.

perhaps this is why mozilla fires watch without checking if the values are equal? it leaves it to the watcher to determine what should count as being equal since it would be difficult to determine what counts as being equal in all cases.

comment:16 in reply to:  15 Changed 9 years ago by John Hann

i promise this is not a desperate attempt to be more persistent ;)

and i promise i'm just trying to get to the root of the issue, not be persistent ;)

Is this something we can just document? i.e. embed an example in the comments:

// dates must be stored as datestamps or strings in order to detect state changes:
var state = new dojo.Stateful();
state.watch('d', function (prop, old, value) {
    console.log('new value for d: ', value);
});
var date = new Date();
state.set('d', date.valueOf());

date.setDate(12);
state.set('d', date.valueOf());

fwiw, i curse at least once a week because dates can't be expressed as literals in javascript (or json). Dates are ALWAYS a special case because that's missing from javascript (and other languages too).

of course, this same problem would also apply to arrays and objects as values. there are different design decisions a developer could make which might help alleviate this problem (the parent object could try to watch all properties of it's children - as long as they were Stateful) but those add more complexity and wouldn't work for Dates

I would argue that it's the developer's responsibility to listen at the correct level. Dates, as I said, are an unfortunate special case every friggin time. (are you sensing my frustration with dates?)

i'm open to hearing about designs that take all of this into account). i'm concerned that having Stateful decide what counts as equality limits a developer's options.

If we're done with the edge cases, then adding an equality test method could be a good solution:

_areValuesEqual: function (value1, value2) {
    return value1 === value2 || isNaN(value1) && isNaN(value2);
}

There's no way to solve the date issue with an equality test method -- not unless you're also caching datestamps.

i came to this decision by wondering how many questions with examples like this would i think will be posted to the mailing list.

excellent point. I will back down on this ASAP if you tell me that this change is going to cause more problems than it solves.

comment:17 Changed 9 years ago by ben hockey

wrt to getting to the root of the issue. from the perspective of dojo, kris has said that he was following the mozilla/seamonkey api. so, wanting to see if i can get closer to the root of the issue, i dug around a little in the seamonkey code to see if there might be any hints as to why it is the way it is. maybe i would find a comment that would show me why watch is called when the value is the same or maybe i would see a reference to a bug discussing it. i didn't find anything so enlightening :)

it seems that watchpoints are basically property setters that call watch handlers. this might mean that there was likely no decision about whether or not watch should be fired when the value is the same. they probably behave the way they do because it was the most convenient way to implement them - this is also why adding a watchpoint adds a property, it's because it needs the property to exist to be able to wrap the setter. watchpoints simply wrap the setter for the object and blindly fire watch handlers whenever they are called. anyhow, for me to think that the behavior might be intentional is probably reading too much into it - convenience was probably the driving force and maybe there was some thought about efficiency which led to avoiding a check for equality (but i doubt it).

as for dates, i curse them too but they are just one example. i chose dates because the solution would not be "turn the date into a dojo.Stateful object." an array example would be:

var a = [1,2,3];

state.set('arr', a);  // fires a watch

a.push(4);
state.set('arr', a); // no watch fires even though the value is 'different'

var b = [1,2,3,4];
state.set('arr', b); // watch fires even though the 'same' value was set

i don't know for sure that the change is going to cause more problems than it solves. honestly, i would defer to bill as more of an authority on what is likely to cause us more headaches to support. he has much more history with dojo than i do and really my reason for mentioning that part of my thought process was to see if i could induce a comment from someone else about what they thought.

again, i'll quiet down now since i've said more than enough to make my point and i'll live with whatever decision is made. this has probably taken more cycles from both of us than it's worth but feel free to respond - i don't mean to deny you that courtesy by saying that i'm going to quiet down.

comment:18 Changed 8 years ago by bill

Milestone: 1.6tbd

Note that [22883] was accidentally rolled back in [23032], so this didn't make it into the 1.6 release.

comment:19 Changed 7 years ago by ben hockey

Keywords: needsreview added
Priority: highlow

comment:20 Changed 3 years ago by dylan

Keywords: needsreview removed
Milestone: tbd1.12
Resolution: patchwelcome
Status: reopenedclosed

I think that given that we have not discussed this in 5 years, we'll close as patchwelcome. We're already addressing this better for 2.x.

Note: See TracTickets for help on using tickets.