Array Stores get Converted to Objects When Retrieving State

[ibitcy]

Array Stores get Converted to Objects When Retrieving State

[ruslanchek]

This happens because each store state object passes a deep freeze before being stored. This is needed for future comparison of the states. (see setState method in the sources). So Array object is an object anyway in JavaScript with its own properties by its nature. And Store stores those properties as-is.

[abgier-avraha]

After trying out your deepFreeze function on an array I can see that it still preserves the Array.isArray status as true and that Object.isFrozen return true.

const frozenArr = deepFreeze(arr);
console.log(Array.isArray(frozenArr), Object.isFrozen(frozenArr), frozenArr);

I think the reason arrays don't work is actually because of this object spread syntax here which is turning arrays into objects.

react-stores/src/Store.ts

Line 205 in 9410910

const updatedState = this.deepFreeze({ ...prevState, ...newState });

I think after changing that locally, I believe array stores started working properly. If this is not something you would like the library to support then I can close the issue otherwise I can prepare a pull request. Perhaps the generic definitions can be update as well to extend only the supported storable types?

[ruslanchek]

It is better to put your array into a property. like so {items: []}.
PS. I never tried it that way. Because this is an anti-pattern for storing and transferring data objects :-)

[abgier-avraha]

I think I understand what you mean but in this case I just wanted my store to be a simple API cache for whatever format the data would be coming in as. I didn't plan on accessing it directly. After looking at this again, it seems to be a problem outside the persistence layer. Arrays seems to be transformed in this way regardless of whether you're persisting data or not. I just wasn't aware of this behaviour.

[ruslanchek]

The spread syntax, of course! Would you mind fixing this and write a test then PR?

[abgier-avraha]

Sure, I'll give it a go tomorrow.

[abgier-avraha]

Going through the setState code and demo project, I've encountered a significant difference of the setState implementation than I initially expected.

I thought that setState worked like the current react useState hook implementation where the new state would overwrite the old state instead of merging with the previous state. But it turns out that this package's implementation is similar to React's setState for class components. When you would call setState in a class component and the argument you passed was a partial object, it would merge it with the current state object. When wasn't an object type, it would just overwrite the previous state instead of attempting a merge.

Considering that there are runtime errors in the development environment to explicitly forbid non objects being used within the state then I suggest this issue gets resolved in another way. Perhaps in another larger issue about how the setState implementation could benefit by allowing for more types and just converging on a similar implementation to setState in React class components since it also does object merging.
https://github.com/ibitcy/react-stores/blob/master/src/Store.ts#L67

I don't mind using the library as is with the current object merging behavior because it seems too large of a design change at the moment to allow for more types. And while I do like how React's useState hook updates state without merging, I'm assuming from the code above that this was not the goal of this library.

So I'm okay closing the issue here.