Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stateful fixes #58

Closed
wants to merge 10 commits into from

Conversation

Projects
None yet
4 participants
@jandppw
Copy link
Contributor

commented Feb 21, 2014

I have some gripes with Stateful. Here are some tests that demonstrate the issues, and fixes in Stateful.

  • the new value reported in callbacks is not the actual new value of the instance
  • callbacks are also called when the property didn't change
  • we need a definition of change and no change

I would like feedback on this.

Does it make any sense to continue on this path?

jandppw added some commits Feb 21, 2014

Stateful has a problem. The new value reported to a watch-callback sh…
…ould be the value get returns at that moment, and it isn't.
Wikipedia says aux comes after foo, bar and baz. http://en.wikipedia.…
…org/wiki/Foobar

And commit these annoying whitespace fixes, so that I am not bothered by them again.
Now we get the actual new value of the property a tad more early, and…
… only call _watchCallbacks when the new value differs from the old one, in set and _changeAttrValue.

We could move this entire decision taking into _watchCallbacks, instead of doing it twice in set and _changeAttrValue, but that would change the signature and behaviour of _watchCallbacks.
The current semantics of _watchCallbacks is better. This allows other classes to "bypass" the whole shebang, and just "send events", as some existing usages actually do.
_watchCallbacks is not hindered by any intelligence. It just dumbly calls callbacks, which is ok that way.
No that we're at it, we best also expand the definition of "being the…
… same". The equals method here is copied from dojox/mvc/sync.

Somebody should describe this, make this "equals" method and interface of something.

Now we still need some tests for the new equality. And then some documentation.
@csnover

This comment has been minimized.

Copy link
Member

commented Feb 21, 2014

These are definitely totally legit concerns, but I think that any such changes to the Stateful API will break backwards compatibility, so would not be allowed in the 1.x branch. There is a lot of awareness of the problems you’ve reported in Stateful and they will definitely be fixed in Dojo 2. Maybe you’d like to help on some of that instead? I’ll let someone else chime in to confirm/deny what I’ve said here.

@jandppw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2014

I understand your concern, of course. That's the main reason why I'm seeking feedback. It does not make sense to continue the effort if it isn't possible to integrate it. For our projects, we created a subclass that does most of these things.

On the other hand, the 2 issues mentioned here (reported newValue is not necessarily the new value, and only sending events when the value actually changes - with a b) issue of defining "change") are the issues that might not break backward compatibility.

There is no documentation that says that the reported newValue is per se the value given to the setter, and the naming of the arguments in all documentation suggests that it should be the actual new value. The current behaviour, at least to me, is a bug. And there will be little or no code that depends on the delta behaviour. The new behaviour just opens the way for dojo/docs#99, which was always intended -- there is little other reason for having custom setters and getters.

Not sending events when the property doesn't change shouldn't break any code. It would just optimise existing applications, resulting in less "updates" and "rerendering". A watcher that is just interested in "set was called" is unlikely, and is better off with another mechanism (aspect.after perhaps).

The equals-thing is exactly what is used in other places too, most markedly in dojox/mvc. Therefor, it should result in no or few surprises.

(There are other concerns, that for sure would break backward compatibility, and would require a "new" interface. E.g. the need for "transactional change notification": the callbacks should be called once for a complex transactional change for "*"-watchers, not once for each property. But that results in a different change reporting format, that supports combined change descriptions (easy peasy: {foo: "new foo", bar: "new bar}, or better, {foo: {old: "old foo", fresh: "new foo"}, bar: {old: "old bar", fresh: "new bar"}. It is clear that there is no good way to do this while keeping the existing callbacks working.
There should be a good way to report changes in to-many associations, not only for singular properties. This probably links to store.observe ... and this probably isn't the forum for that.
There are concerns that can be handled by Stateful for reference properties, such as arrays, ...
There are issues concerning derived properties, ...)

All this just to illustrate that the 2,5 issues presented here might be those that can be considered bug fixes, not changes to semantics.

And I would like to peek at what is going on with 2.0, fersure. But is that code available? Because I haven't stumbled across it yet.

Anyway, for this pull request, please let me know whether you want it in 1.x, or not. If so, I will add some more tests, and update the documentation. Otherwise, just reject the pull request for now. The internet doesn't forget ;-).

@dylans

This comment has been minimized.

Copy link
Member

commented Apr 9, 2014

Created ticket https://bugs.dojotoolkit.org/ticket/17856 to track this, we will not get to this for 1.10

@dylans dylans added this to the 1.11 milestone Apr 9, 2014

@wkeese

This comment has been minimized.

Copy link
Member

commented Apr 9, 2014

I tend to agree with csnover wrote in the first comment, that these changes can't happen until 2.0.

the new value reported in callbacks is not the actual new value of the instance

I guess that's debatable which behavior is correct, but you can get the "actual new value" by referencing this.foo.

callbacks are also called when the property didn't change

That's how dijit works, and it's how Stateful used to work (see bb96d9d) until it was accidentally changed in fd62e05. It was also the topic of a lot of religious debate a few years ago, centered around metaphysical questions like "how do we know if two values are the same?".

@dylans dylans removed this from the 1.11 milestone Apr 10, 2014

@dylans

This comment has been minimized.

Copy link
Member

commented Sep 11, 2015

Given that this won't be able to land in 1.x, and the work for 2.x is quite different (e.g. https://github.com/dojo/core/blob/master/src/observers/ObjectObserver.ts ), I'm going to close this one out. @jandppw , please let us know if you have any feedback on how we're tracking property changes with 2.x.

@dylans dylans closed this Sep 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.