KidkArolis / tiny-atom

Pragmatic state management.
MIT License
116 stars 10 forks source link

Rethink deepMerge, it's not a success pit #57

Closed KidkArolis closed 5 years ago

KidkArolis commented 6 years ago

The current default deep merge behavior can lead to subtle, hard to notice issues.

When calling set({ nested: { a: 1 }}), you usually don't consider that the rest of the nested attributes remain. Often it's convenient, but sometimes it will lead to bugs.

Ideally there's a way to make this work where it's more explicit, but still convenient. Some ideas:

KidkArolis commented 6 years ago

Consider this API:

// replace entire state
set(val)
// replace a key
set(key, val)
// replace a val inside a nested key
set(keys, val)

// get entire state
get()
// get state at key
get(key)
// get state at neste keys
get(keys)
KidkArolis commented 6 years ago

Consider this API:

set
setIn
updateIn
get
getIn
merge
KidkArolis commented 6 years ago

Ok, quite liking the option of:

set({}) assigns to the state shallowly (like setState does)
swap({}) replaces it entirely

Then could also consider (but most likely not needed at all!) adding:

set(['nested', 'key'], {})
swap(['nested', 'key'], {})
KidkArolis commented 6 years ago

Or

set - shallow
merge - deep
swap - replace
KidkArolis commented 6 years ago

Today, leaning towards:

// useful as the base operation, useful with immutable/immer
set() - replaces the state
setIn() - replaces the state at some path

// very convenient for merging updates in
merge() - merges the state
mergeIn() - merges the state at some key

// kind of for completeness
update() - updates the state via fn
updateIn() - updates the state via fn at some key

So no more deep merging, I considered allowing deep merging with patch or patchIn, this way you can sort of say, take this deeply nested object and sort of patch it into the state. But it's quite confusing how deep merging treats objects differently from other data structures and primitives. So actually, maybe, it's better to just not offer it (one can still use it if really useful for a particular scenario via lodash, etc.). You can achieve most of what deep merge offers via mergeIn or updateIn. I'll just have to try and see how the code looks like after this change, might not "feel right", it felt quite right with deepMerge.

All this also means that we can remove options.merge! Since that's now controlled on the action side more so than on the atom internals.

KidkArolis commented 5 years ago

Done so on master, we now have set and swap. Options .merge is actually still relevant for Immutable use case when fusing(), to merge the states. So I should probably bring that back. Because evolve is middleware for actions, merge is middleware for set.

KidkArolis commented 5 years ago

I won't readd merge, it's a bit of a yagni. If you use Immutable, you can workaround any number of ways. First of all, fuse in general is not that useful as is, so might be changed in the future. Second, you can fuse new actions with fuse, but then manually add extra state with atom.swap, etc.