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

[question]diff available to observers? #92

Open
davidmaxwaterman opened this issue Jul 11, 2017 · 11 comments
Open

[question]diff available to observers? #92

davidmaxwaterman opened this issue Jul 11, 2017 · 11 comments

Comments

@davidmaxwaterman
Copy link

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
Copy link
Author

Someone pointed me at a closed issue:

#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
Copy link

alxdnlnko commented Jul 12, 2017

+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
Copy link

@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
Copy link
Owner

tur-nr commented Jul 12, 2017

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
Copy link

@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
Copy link
Author

davidmaxwaterman commented Jul 12, 2017

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
Copy link
Author

@jrunning
Copy link

@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:

// type of array, work out splices before setting the value
if (property.type === Array) {
// compare the splices from a previous copy
previous = prevArrays[property.name] || [];
splices = Polymer.ArraySplice.calculateSplices(value, previous);
// keep for next compare
prevArrays[property.name] = value ? value.concat() : [];
}
// set
element.notifyPath(property.name, value, !property.readOnly);
// notify element of splices
if (splices.length) {
element.notifySplices(property.name, splices);
}

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

@davidmaxwaterman
Copy link
Author

Hrm, interesting, thanks.

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

@tur-nr
Copy link
Owner

tur-nr commented Sep 20, 2017

@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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants