bitovi / ylem

Add Observable View-Models to React components
https://bitovi.github.io/ylem/
MIT License
42 stars 2 forks source link

Ylem splices array-like objects passed as props #181

Open mikedane opened 6 years ago

mikedane commented 6 years ago

Small bug in observable-component.js on line 95

canReflect.splice(canKey.get(observable, key), values.index, values.deleteCount, values.insert);

This code seems to be modifying the original prop that was passed instead of replacing it with the new prop. This was initially intended to affect arrays only, and in the case where an array is being passed it should still behave this way. If however, an array-like object is passed this is not the desired behavior as that array-like object may have additional properties of interest to the child which need to be overwritten.

This should be re-written to simply pass the new prop when dealing with an array-like object, and to behave the same when dealing with a normal array.

mikedane commented 6 years ago

One point of discussion that has been brought up is whether or not splicing is actually the best solution for arrays. It doesn't work for array-like objects because they can have other info besides the list of items. But arrays can also store other info, in fact identity itself is info. Someone might be testing if oneArray === anotherArray

Would it be better to just simply pass the new array in every case instead of splicing?

@justinbmeyer @christopherjbaker

This also brings up another question which is should we even be relying on the diff to determine whether or not a new prop is passed. Imagine a situation where you have an array like object that is storing an extra field like filter. If all of the items in that array-like object stay the same and just the filter changes, that new prop will not even be passed down to any children because ylem thinks that nothing has changed (i.e a splice doesn't need to happen), when in reality it has changed.

I'm running into this problem with the TodoMVC example right now