AriaFallah / mobx-store

A data store with declarative querying, observable state, and easy undo/redo.
MIT License
282 stars 9 forks source link

`localStorage.setItem` inside `read`? #19

Closed dmitriz closed 8 years ago

dmitriz commented 8 years ago

I can see why you write

localStorage.setItem(source, '{}')

inside your read here but it still feels dangerous as you would expect read accessor to be idempotent side-effect-free.

Perhaps moving it outside into some kind of setDefault function?

EDIT. It is actually idempotent but not side-effect-free.

AriaFallah commented 8 years ago

Good point. Actually I don't even think that's necessary anymore so I'll roll out a change to fix that. Thanks! It's a relic of me copying lowdb initially.

AriaFallah commented 8 years ago

@dmitriz

What do you think of this then? Should I make people explicitly set the store? https://github.com/AriaFallah/mobx-store/blob/master/src/index.js#L10a

AriaFallah commented 8 years ago

https://github.com/AriaFallah/mobx-store/commit/d0b9e91380f31ee0169e05a76057b66f4907e65e

dmitriz commented 8 years ago

If I understand it right, the object db sets the default state, which is somewhat confusing to expect from an object ;)

Maybe separately create it and then setDefault? These feel like 2 separate responsibilities.

Ideally you want to make it easy for people to set their store, but at the same time have clever defaults.

AriaFallah commented 8 years ago

@dmitriz Basically if you do

store('test').push(1)

it'll create store('test') for you instead of throwing store('test').push is not a function.

Currently you can set the default state when you first make your store

const store = mobxstore({ test: [1, 2, 3] })

The proposed change is to make

store('test').push(1)

break and first you have to do something like

store.set('test', [])
// now we can do
store('test').push(1)
dmitriz commented 8 years ago

Does store('test') create or access the store? From its name, I would expect the second.

How about createStore('test') to create and store('test') to access? So I can be safe to know, store is side-effect-free, and createStore will have its explicitly declared side-effect responsibility.

Then createStore will overwrite the store, which I don't expect from an accessor.

So yes, if the store test does not exist, and you do set('store').push(1), I would expect it to throw exception that I can quickly see rather than do some magic silently for me.

That is the general complaint about frameworks "doing magic" vs libraries giving enduser more control.

Also store.set('test', []) feels like applying write accessor to the store already defined. I still prefer createStore because it is impossible to confuse for anything else:

myDefaults = []
emptyStore = createStore('test')
defaultStore = createStore('test', {defaults: myDefaults }) 

emptyStore.push(1) // error: undefined is not an array
defaultStore.push(1) // [1]