cefn / watchable

Repo for @watchable/store and supporting packages.
MIT License
5 stars 1 forks source link

Allow a state to be a primitive #35

Closed marcusletric closed 10 months ago

marcusletric commented 1 year ago

As of now a State object is an extension of the RootState type that aliases an object. This imposes the restriction that a State cannot hold a primitive.

However, when working with a complex application, utilising partitions to hold component state might induce the desire for such functionality:

const appStore = createStore({});
const componenStatePartition = createStorePartition(appStore, "billingComponentState"));
const loadingState = createStorePartition(componenStatePartition, "isLoading"));

componentStatePartition.write(true) <- Typescript error parameter is not an `object`

I also think that by philosophy a State should not be restricted to be an object, instead the PartitionableState type should be restricted to an object to allow partitioning.

With all above and the help from the usePartition hook from the @watchable-react/store-hooks package, it would allow developers to write this concise code in their react app:

const appStore: Store<AppState> = useAppStore();
const componenStatePartition = useMemo(() => createStorePartition(appStore, "billingComponentState"), []);
const [snackbar, setSnackbar] = usePartition(componenStatePartition, "snackbar");
const [showRemoveMopDialog, setShowRemoveMOPDialog] = usePartition(componenStatePartition, "showRemoveMopDialog");

setShowRemoveMOPDialog(false);

Hence I would suggest to amend RootState with union of primitives and restrict PartitionableState to be an object of RootState values.

I can't think of any problems this should cause with the implementation of Watchable, Store, WatchableState or DefaultStorePartition but let me know if I'm missing something.

cefn commented 10 months ago

We've been discussing this outside this thread, but I should echo the conclusions drawn in this thread for later archaeology.

A signature like React's useState is desirable, but should probably not be based on Store partitions, for reasons outlined here and here.

In summary partitioned stores are an overhead you would opt into for very specific benefits (not just having a read/write signature) and they have some costs too.

With luck your more specific requirement (to read and write sub-states) will be fulfilled by https://github.com/cefn/watchable/pull/40

A related piece of work is this use of paths not just keys to allow setting+getting even 'deep' descendants. But that is very early days.

Although we have identified a slightly narrower scope for this feature request, I have no specific objection to broadening RootState, so I have included this change in the MR.

export type RootState = unknown;

The use of an object RootState was intended as a hint about the purpose of Watchable - e.g. that objects and arrays are non-atomic and not immutable by default in Typescript/Javascript, and that care needs to be taken when using them for state. When using primitives for state you have no such issue - assignment is safe, and Object.is directly provides change detection.

As in this case, the use of anything but an object or array as the RootState is a code smell that probably the approach needs to be reconsidered. However, I can also imagine ways of exploiting and wrapping watchable where it's just easier NOT to differentiate between non-atomic and atomic (primitive) values, rather than having them as special cases, hence loosening RootState.

marcusletric commented 10 months ago

I was not aware of the overhead of store partitions, but thanks for the explanation, I can absolutely agree with the reasons not to use it as a store branch setter/getter, and yes useStateProperty does serve all the needs I had in mind.

cefn commented 10 months ago

Closed by https://github.com/cefn/watchable/pull/40