Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#12399 closed defect (invalid)

Difference in dijit._WidgetBase watch() callbacks and dojo.Stateful watch() callbacks.

Reported by: rahul Owned by: anonymous
Priority: high Milestone: tbd
Component: Dijit Version: 1.6.0
Keywords: Cc: Kris Zyp
Blocked By: Blocking:

Description

In dojo.Stateful, watch() callbacks are issued on set() regardless whether the new value equals the old.

In dijit._WidgetBase, some watch() callbacks are only issed on set() if value !== oldValue.

I can understand the rationale, since for properties like "value", "style", "class" etc. one wouldn't want to issue such callbacks for looping or performance's sake.

However, some widgets may need to be informed any time a set() call is made for specific properties, even if the old and new values are equal, just like they'd be informed in the dojo.Stateful class implementation.

A small change to the _set() implementation will enable widgets to list such properties. Please see forthcoming patch.

Widgets can thereby indicate they'd like to get watch() callbacks on all set() calls for specific properties as follows -- here, my.Watcher wants to get watch() calls on all set()s on "foo" and "bar" properties:

dojo.declare("my.Watcher", "dijit._WidgetBase", {

  foo: "...some initial value...",

  bar: "...",

  _enableCallbacksIfEqual: { foo: true, bar: true },

  // rest of my.watcher impl
});

Attachments (2)

_WidgetBase-watch-pattern.patch (671 bytes) - added by rahul 8 years ago.
Patch allowing widgets to list specific properties that need all watch() callbacks.
ignoreIdentical.diff (3.2 KB) - added by ben hockey 8 years ago.
patch for Stateful.watch to allow an ignoreIdentical param. also removes ignoreCatchAll remnants which looked unused.

Download all attachments as: .zip

Change History (15)

Changed 8 years ago by rahul

Patch allowing widgets to list specific properties that need all watch() callbacks.

comment:1 Changed 8 years ago by ben hockey

see http://bugs.dojotoolkit.org/ticket/11738 for some related discussion

comment:2 Changed 8 years ago by rahul

Thanks, I read the related discussion. As in some examples in that discussion, there are cases where it'd be useful to get watch() callbacks on all set() calls. This patch enables developers to make that informed choice in the context of widget properties, which is very useful.

comment:3 Changed 8 years ago by bill

Component: GeneralDijit
Resolution: wontfix
Status: newclosed

Sorry, I'm not keen on adding a flag to control behavior for a number of reasons:

  • you can connect to set() to get the functionality you want
  • setting that flag would probably cause problems for widgets that internally watch() themselves or other widgets
  • [22883] modified dojo.Stateful to not report calls to set() where the old and new value were the same. AFAIK, besides corner cases like NaN, and maybe undefined vs. null vs. "", Stateful doesn't report "changes" where the new and old value are the same
  • IIRC, widgets internally end up calling set() a lot, with the same value. In that case, calling code would get unwanted watch() notifications

comment:4 Changed 8 years ago by rahul

Resolution: wontfix
Status: closedreopened

I ask that you reconsider for the following reasons:

  • This conflates lexical and value spaces. As in some examples in referenced discussion, this is a real-world issue biting number of usecases.
  • This is a good example of premature optimization. The library shouldn't make the choice on the semantics of equality of property values WRT notifications.
  • If the library does indeed make such a choice, atleast it should attribute intelligence to users by allowing them to make conscious decisions to override it. If users want to revert to previous dojo.Stateful behavior, no means have been provided to do so.

I don't agree with your bulleted reasons for wontfix'ing this, in the same order as your bullets:

  • This is a red herring. I want to use Stateful in all its elegance, not just make things work. I don't want to augment it with connect() and hybridize the implementation. Sure, there are other ways to do similar things, but impls of basic interfaces like Stateful need to cater widely.
  • Setting the flag is well-informed application choice. For example, it'd be set for new properties that require this behavior.
  • I also disagree with the same change made to dojo.Stateful, but I wasn't paying attention around [22883] to win by persistence -- even then this didn't seem clear cut.
  • This patch is backwards compatible. Not a single extra watch() call is made by default, there will be no unwanted/extra notifications.

In summary, please accomodate usecases (and users) where they can explicitly pick properties to get previous dojo.Stateful behavior without changing any existing behavior at all; especially given that the Stateful interface is so fundamental and pervasive -- it really needs to cater to widest audiences.

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

Replying to rahul:

I ask that you reconsider for the following reasons:

  • This conflates lexical and value spaces. As in some examples in referenced discussion, this is a real-world issue biting number of usecases.
  • This is a good example of premature optimization. The library shouldn't make the choice on the semantics of equality of property values WRT notifications.
  • If the library does indeed make such a choice, atleast it should attribute intelligence to users by allowing them to make conscious decisions to override it. If users want to revert to previous dojo.Stateful behavior, no means have been provided to do so.

I don't agree with your bulleted reasons for wontfix'ing this, in the same order as your bullets:

  • This is a red herring. I want to use Stateful in all its elegance, not just make things work. I don't want to augment it with connect() and hybridize the implementation. Sure, there are other ways to do similar things, but impls of basic interfaces like Stateful need to cater widely.
  • Setting the flag is well-informed application choice. For example, it'd be set for new properties that require this behavior.
  • I also disagree with the same change made to dojo.Stateful, but I wasn't paying attention around [22883] to win by persistence -- even then this didn't seem clear cut.
  • This patch is backwards compatible. Not a single extra watch() call is made by default, there will be no unwanted/extra notifications.

In summary, please accomodate usecases (and users) where they can explicitly pick properties to get previous dojo.Stateful behavior without changing any existing behavior at all; especially given that the Stateful interface is so fundamental and pervasive -- it really needs to cater to widest audiences.

fwiw, i strongly agree with rahul. i tried to argue the same case before [22883] but was not successful in presenting it as clearly. something as fundamental as Stateful should cater to the widest audiences.

comment:6 Changed 8 years ago by bill

Resolution: invalid
Status: reopenedclosed

Opinions noted, but (as you mentioned) we already discussed this at length in #11738 and decided to make watch() only report changes to values. At this point, I'm not interested in changing the semantics of watch(), especially not in making the behavior vary depending on the attribute name.

In any case this ticket is invalid, there's no significant difference between watch() in dijit._WidgetBase and dojo.Stateful, they both only report changes.

comment:7 Changed 8 years ago by rahul

No, the above is factually incorrect as follows:

  • The current dojo.Stateful behavior in trunk reports all set()s
  • The dojo.Stateful behavior in v1.5 reports all set()s
  • The dojo.Stateful behavior in v1.6 reports all set()s

Thus, there is absolutely a significant difference.

When I have a patch as discussed in IRC meeting yesterday, I'll reopen this ticket.

comment:8 Changed 8 years ago by ben hockey

Cc: Kris Zyp added

given that dijit's watch and dojo.Stateful's watch behave differently then i think we probably all agree that *something* needs to change to make these consistent. i'd like to suggest an approach that might be something agreeable to all.

would it be reasonable to change the signature of watch in dojo.Stateful to take a 3rd parameter called ignoreIdentical? if truthy, this callback being added would not be called if the oldValue and newValue were identical (ie oldValue === newValue).

in this way, watch(prop, callback)

  • behaves the same as mozilla's watch which is to call the callback every time the property is set,
  • existing code using the current behavior of dojo.Stateful does not change and so we are backwards compatible

and watch(prop, callback, true) will only call callback if oldValue !== newValue and in order for dijit to backwards compatible with its current release, dijit changes to use this 3rd param internally when it expects identical values to be ignored.

i believe this approach will give us the best of backwards compatibility and provide complete flexibility going forward. thoughts?

Changed 8 years ago by ben hockey

Attachment: ignoreIdentical.diff added

patch for Stateful.watch to allow an ignoreIdentical param. also removes ignoreCatchAll remnants which looked unused.

comment:9 in reply to:  8 Changed 8 years ago by Rawld Gill

Replying to neonstalwart:

This seems like a very good solution to me. My preference would be for the 3rd param to be "signalIdentical" with obvious semantics. I understand this breaks back-compat on code, but keeps it on docs.

comment:10 in reply to:  4 ; Changed 8 years ago by Rawld Gill

Replying to rahul:

I ask that you reconsider for the following reasons:

  • This conflates lexical and value spaces. As in some examples in referenced discussion, this is a real-world issue biting number of usecases.

I don't understand what you mean by "lexical" and "value" spaces. If you mean 0.5 is different than "0.5", see next.

  • This is a good example of premature optimization. The library shouldn't make the choice on the semantics of equality of property values WRT notifications.

Well, what operator are you using? I agree that 0.5 is not equal to "0.5". But, if operator=== returns true, the two operands are equivalent by any reasonable definition of equality.

  • If the library does indeed make such a choice, atleast it should attribute intelligence to users by allowing them to make conscious decisions to override it. If users want to revert to previous dojo.Stateful behavior, no means have been provided to do so.

The backcompat argument is strong. Ben's suggestion is a nice soln.

I don't agree with your bulleted reasons for wontfix'ing this, in the same order as your bullets:

  • This is a red herring. I want to use Stateful in all its elegance, not just make things work. I don't want to augment it with connect() and hybridize the implementation. Sure, there are other ways to do similar things, but impls of basic interfaces like Stateful need to cater widely.

You've called this suggestion "a red herring" and "cop out" (in dojo-meeting). I have yet to hear an engineering argument why connect is bad. connect was designed to signal on an application of a function. That's what you need. Why is it not elegant?

  • Setting the flag is well-informed application choice. For example, it'd be set for new properties that require this behavior.

The proposed patch causes all watchers to be subject to the value of the flag. So, one piece of machinery gets to control how watch behaves towards other (by definition, orthogonal) machinery also connecting via watch. That's bad design. It becomes a monolith.

Ben's suggestion is much better.

  • I also disagree with the same change made to dojo.Stateful, but I wasn't paying attention around [22883] to win by persistence -- even then this didn't seem clear cut.
  • This patch is backwards compatible. Not a single extra watch() call is made by default, there will be no unwanted/extra notifications.

In summary, please accomodate usecases (and users) where they can explicitly pick properties to get previous dojo.Stateful behavior without changing any existing behavior at all; especially given that the Stateful interface is so fundamental and pervasive -- it really needs to cater to widest audiences.

The proposed patch does not cater to me. What evidence is there that it caters to the widest audience?

comment:11 in reply to:  10 ; Changed 8 years ago by rahul

Replying to rcgill:

Replying to rahul:

I ask that you reconsider for the following reasons:

  • This conflates lexical and value spaces. As in some examples in referenced discussion, this is a real-world issue biting number of usecases.

I don't understand what you mean by "lexical" and "value" spaces. If you mean 0.5 is different than "0.5", see next.

  • This is a good example of premature optimization. The library shouldn't make the choice on the semantics of equality of property values WRT notifications.

Well, what operator are you using? I agree that 0.5 is not equal to "0.5". But, if operator=== returns true, the two operands are equivalent by any reasonable definition of equality.

Apps and values aren't always context free. For example, on a mobile device, the choice "nearest" depends on when it was set() -- and even when set repeatedly may resolve to different locations. OTOH, the string "tomorrow" set() today may be equal in value to the string "today" set() tomorrow. These are fictitious examples, but to think about equality in terms of === or any other operator is a constraining approach, better to leave it to the app to decide.

  • If the library does indeed make such a choice, atleast it should attribute intelligence to users by allowing them to make conscious decisions to override it. If users want to revert to previous dojo.Stateful behavior, no means have been provided to do so.

The backcompat argument is strong. Ben's suggestion is a nice soln.

I don't agree with your bulleted reasons for wontfix'ing this, in the same order as your bullets:

  • This is a red herring. I want to use Stateful in all its elegance, not just make things work. I don't want to augment it with connect() and hybridize the implementation. Sure, there are other ways to do similar things, but impls of basic interfaces like Stateful need to cater widely.

You've called this suggestion "a red herring" and "cop out" (in dojo-meeting). I have yet to hear an engineering argument why connect is bad. connect was designed to signal on an application of a function. That's what you need. Why is it not elegant?

If I say doing something will break Foo, and I hear back there is shiny Bar that does similar things, we are not talking about the same thing. Unless we are talking about the core issue (why break Foo), its a distraction.

  • Setting the flag is well-informed application choice. For example, it'd be set for new properties that require this behavior.

The proposed patch causes all watchers to be subject to the value of the flag. So, one piece of machinery gets to control how watch behaves towards other (by definition, orthogonal) machinery also connecting via watch. That's bad design. It becomes a monolith.

Ben's suggestion is much better.

If you were in the meeting then you also know I'm glad to have better approaches used here :-)

  • I also disagree with the same change made to dojo.Stateful, but I wasn't paying attention around [22883] to win by persistence -- even then this didn't seem clear cut.
  • This patch is backwards compatible. Not a single extra watch() call is made by default, there will be no unwanted/extra notifications.

In summary, please accomodate usecases (and users) where they can explicitly pick properties to get previous dojo.Stateful behavior without changing any existing behavior at all; especially given that the Stateful interface is so fundamental and pervasive -- it really needs to cater to widest audiences.

The proposed patch does not cater to me. What evidence is there that it caters to the widest audience?

It enables more usecases than before.

comment:12 in reply to:  11 ; Changed 8 years ago by Rawld Gill

Replying to rahul:

Replying to rcgill:

Replying to rahul:

Apps and values aren't always context free. For example, on a mobile device, the choice "nearest" depends on when it was set() -- and even when set repeatedly may resolve to different locations. OTOH, the string "tomorrow" set() today may be equal in value to the string "today" set() tomorrow. These are fictitious examples, but to think about equality in terms of === or any other operator is a constraining approach, better to leave it to the app to decide.

OK. I think I understand what you are getting at. I'll say it back to make sure...

set() may not be simply setting an atomic value. For example set(A, x) could cause some sort of computation that would affect the value of get(A). It would be possible for set(A, x) to cause get(a) to return something other than x.

If I have understood:

  • This observation does not change my opinion about the behavior of watch.

[snip]

If I say doing something will break Foo, and I hear back there is shiny Bar that does similar things, we are not talking about the same thing. Unless we are talking about the core issue (why break Foo), its a distraction.

No. You have been told there is a shiny Bar that will fix foo. And I still haven't heard why the shiny bar won't work. I remain open to change my opinion and admit my error if you can do this.

Note also, that to make stateful.watch signal on no-ops will break other code. The reason for an API decision can't be "because it breaks my code".

The proposed patch does not cater to me. What evidence is there that it caters to the widest audience?

It enables more usecases than before.

It does? Because you say so? I'm going to stop arguing this point; it was a bit of an unfair question. It's a contest where a winner can't be proven. Besides, if the entire world is doing it wrong, it still may be wise to do it right (whatever is adjudicated right or wrong).

comment:13 in reply to:  12 Changed 8 years ago by rahul

Replying to rcgill:

Replying to rahul:

Replying to rcgill:

Replying to rahul:

Apps and values aren't always context free. For example, on a mobile device, the choice "nearest" depends on when it was set() -- and even when set repeatedly may resolve to different locations. OTOH, the string "tomorrow" set() today may be equal in value to the string "today" set() tomorrow. These are fictitious examples, but to think about equality in terms of === or any other operator is a constraining approach, better to leave it to the app to decide.

OK. I think I understand what you are getting at. I'll say it back to make sure...

set() may not be simply setting an atomic value. For example set(A, x) could cause some sort of computation that would affect the value of get(A). It would be possible for set(A, x) to cause get(a) to return something other than x.

We're getting closer (I'll skip some nuances for now).

If I have understood:

Cool, though I haven't at above code.

  • This observation does not change my opinion about the behavior of watch.

Lets leave our opinions aside for a brief second or two, and talk state of the code. Do you disagree with the title of this ticket i.e. there is a difference in _WidgetBase and Stateful's watch() callbacks? (in trunk today, in 1.5 or in 1.6, take your pick). Its incorrect to say there isn't a difference (and therefore, this ticket is invalid). This part is not a matter of opinion.

[snip]

If I say doing something will break Foo, and I hear back there is shiny Bar that does similar things, we are not talking about the same thing. Unless we are talking about the core issue (why break Foo), its a distraction.

No. You have been told there is a shiny Bar that will fix foo. And I still haven't heard why the shiny bar won't work. I remain open to change my opinion and admit my error if you can do this.

Others have mentioned semantic differences on the list, but lets be clear, connect() does not fix Stateful. connect() may fix apps that want to continue using the current Stateful behavior. That is very different from fixing Foo itself.

Note also, that to make stateful.watch signal on no-ops will break other code. The reason for an API decision can't be "because it breaks my code".

Users and customers complain when the rug is pulled from under their feet because some behavior changes. Having authored number of libraries, let me add that users are correct here. API decisions are to be made when code is introduced, not some releases down the road.

The proposed patch does not cater to me. What evidence is there that it caters to the widest audience?

It enables more usecases than before.

It does? Because you say so? I'm going to stop arguing this point; it was a bit of an unfair question.

Note I'm still trying to offer a fair response :-)

It's a contest where a winner can't be proven.

Let see:

  • Set A: Class of apps where === makes sense
  • Set B: Class of apps where it doesn't make sense

Today, we enable a union of those in the dojo.Stateful impl. Not so in _WidgetBase.

Besides, if the entire world is doing it wrong, it still may be wise to do it right (whatever is adjudicated right or wrong).

OK, but lets be informed about this. I don't think there are any outright advantages to change Stateful behavior and it would actually preclude Set B above (which is the point of the related thread on the list).

Thanks for your time in looking into this.

Note: See TracTickets for help on using tickets.