tur-nr / polymer-redux

Polymer bindings for Redux.
https://tur-nr.github.io/polymer-redux
MIT License
440 stars 67 forks source link

[question]diff available to observers? #92

Open davidmaxwaterman opened 7 years ago

davidmaxwaterman commented 7 years ago

If I have an observer on a property managed by polymer-redux, it always says everything has changed, probably due to the clone-change-assign methodology I use; in other words, it is replacing everything.

Perhaps the answer to my question is "don't do that" - ie instead of replacing entire parts of the state, just make minimal changes.

To be clear, when I want to add something to an array (in a case in my redux-store.html), I call slice(0) on it and assign that to a variable, push the new item onto that new array, and then assign that new array back on top of the old array. The array is a property of state itself, which I clone using Object.assign(). It all seems quite inefficient, but I think I got that method from somewhere (I wouldn't have come up with that on my own).

Any advice?

davidmaxwaterman commented 7 years ago

Someone pointed me at a closed issue:

https://github.com/tur-nr/polymer-redux/issues/12

that has an example that does not clone the array, but instead pushes the new item on top of the array that is already in state. Is this the 'correct' way?

alxdnlnko commented 7 years ago

+1

Tried array.splices, object.prop and object.* observers. Nothing works: "splices" are always called with undefined (even on array[ind].prop change), because the whole array is new, "myObject.*" is called with path === 'myObject', because the whole object is new, and so on.

Maybe there is some another workflow with immutable data without using observers? Do react-guys have this problem?

alxdnlnko commented 7 years ago

@davidmaxwaterman

that has an example that does not clone the array, but instead pushes the new item on top of the array that is already in state. Is this the 'correct' way?

If you do so, forget about redux :) State must be immutable. But how we can know, what exactly was changed in that array without diff libs - this is the question.

tur-nr commented 7 years ago

Using Redux state should always be immutable, so the change of properties will always be new.

React has no concept of "observing". It also uses a virtual dom to calculate the differences not data equality, which means immutable state is fine.

Have you looked into google/uniflow-polymer?

Can I ask what your use case is for retaining the original array/object. Are you comparing the differences?

alxdnlnko commented 7 years ago

@tur-nr Hi. Just the first case, that came to my head: how to animate items recently added to the array? We don't know, what are the new items, because the whole array is a new object. With mutable data we could use the array.splices observer for this.

davidmaxwaterman commented 7 years ago

Can I ask what your use case is for retaining the original array/object. Are you comparing the differences?

If this question is for me, I'm not entirely sure what you're asking. I don't think I do want the original array/object. I'm not comparing the differences. All I want is what has just been pushed onto the array - ie what is new. At the moment, as items are pushed onto the array, my observer is called repeatedly saying the whole array has changed. Since I know items are pushed, I can assume that the 'top' one is the new one, but there are other times when things change in the array (eg I change a property of one of the objects in the array) when the observer is called, again indicating the whole array has changed. The behaviour I would be expecting is if I did a this.set(myArray.${index}.prop, 'newvalue')...if I do that, then the observer is called with a path myArray.index.prop indicating exactly which item in the array has changed. Unfortunately, that doesn't seem to be acceptable for something stored inside redux.

FWIW, my observer is actually wanting to know when a specific item has been added to the array. As such, each time the observer is called, it does a find() on the array, but only because the path indicates that the whole array has changed. If it could distinguish between the whole array changing and just one item being added (pushed, unshifted, etc), then it would be able to avoid going through the whole array and just check the one that was added - well, not the whole array; just until it found the one it was looking for.

Perhaps my summary and description on this issue isn't quite right, sorry about that.

(edit:I've not checked out uniflow-polymer, but am reading it now, thanks)

davidmaxwaterman commented 7 years ago

Also, this seems quite relevant :

https://www.captaincodeman.com/2017/07/06/managing-state-in-polymer-20-beyond-parent-child-binding

jrunning commented 7 years ago

@davidmaxwaterman You can use Polymer.ArraySplice.calculateSplices for this. The basic pattern looks like this:

static get properties() {
  return {
    notifications: {
      type: Array,
      statePath: 'notifications',
      observer: '_observeNotifications',
    },
  };
}

_observeNotifications(newArray, oldArray) {
  const changes = Polymer.ArraySplice.calculateSplices(newArray || [], oldArray || []);
  console.log('notifications changed', changes);
}

Caveat emptor: Depending on the size of your data, this may have performance implications.

Oddly, this is exactly what polymer-redux used to do:

https://github.com/tur-nr/polymer-redux/blob/23dbbc9c51872a2b43ae1a8a0726be130851bd2c/polymer-redux.js#L25-L38

I'm not sure why @tur-nr took it out.

davidmaxwaterman commented 7 years ago

Hrm, interesting, thanks.

What about complex observers where you don't get an 'oldArray'? Also, Objects?

tur-nr commented 7 years ago

@jrunning the reason why we taken it out is because observing splices of an array is a mutation of the data. Redux doesn't follow the same rules, if the array has a mutation it is a new value.

So we needed a trade off.

A. ~Enforce everyone that uses polymer-redux to mutate arrays in the reducer so that polymer-redux can apply splice calculations.~ B. Allow everyone to use Redux as intended and just set new array.

Now why don't we just calculate the splices like in version v0?

So in Polymer whenever you set a new value for an array you get two notifications; a property value effect and a property mutation effect. A property value effect is simple, heres your new value and heres you old. However the mutations effect after you set a new value for arrays is always undefined regardless the fact if you're using polymer-redux or not. So that's two notifications happened. Now we notify the element of the splices which will cause a 2nd property mutation effect, with the splice information.

So from a developers prospective we have a double mutation effect. IMO it's not efficient and can also lead to issues whenever a Element wishes to do some logic with a mutation observer.

@davidmaxwaterman the point I'm trying to make is that Redux is state management library for immutable data. Therefore if you choose it for your Polymer project, you will lose some functionality that Polymer provides for mutable data, which in this case is observing splices. To overcome this @jrunning example is should do the trick.

davidmaxwaterman commented 7 years ago

OK, I get that. I suppose I'm questioning my choice of using polymer-redux/redux in the first place. I wanted the central state management but wasn't expecting to lose the functionality Polymer provides. As such I intend to switch to using a solution based on this sort of thing which just works the way I already know: https://www.captaincodeman.com/2017/07/06/managing-state-in-polymer-20-beyond-parent-child-binding