AriaFallah / mobx-store

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

Setting properties on nested objects #34

Closed mochacoder closed 8 years ago

mochacoder commented 8 years ago

I feel like I'm just missing something obvious here but I'm having trouble setting a nested object property in a way that mobx-store will retain the history:

If I have the following store:

const store = mobxstore({ todos: { id123: { title: 'test todo', complete: false } } });

The only way I can successfully update the title is if I use something like the following:

store('todos').get("id123").title = "test update todo title";

But it looks like that bypasses the built in mechanisms mobx-store has for doing sets on objects as title is updated in the history and not included as a separate update.

Again seems like I'm missing something obvious but the examples don't show anything nested like this. Thanks for the help.

AriaFallah commented 8 years ago

@tcjustin

Yeah so I should probably document this better, but this is probably the biggest flaw with the library right now.

If you look at this issue https://github.com/mobxjs/mobx/issues/214, MobX currently has no way to do deep observation, and the only solution I have thought of to the problem would make this library horribly complex and bug prone.

For the time being, this means that if you make any changes deeper than one level into the store, the library doesn't receive the change event from the observer and there isn't a good way to undo that change. The only workaround as of now is something like this:

const todos = store('todos')
todos.set('id123', { ...todos.get('id123'), 'title: test update todo title' })

Basically, you have to take a redux-like approach, and make changes in an immutable manner. This way you're changing the top level id123, and it'll be registered in the history.

The MobX roadmap has a time traveling mechanism listed, but for now this is the best I can do with the current undo/redo implementation.

Is that helpful?

mweststrate commented 8 years ago

@ariafallah, did you take a look at the spy api? time travelling will be based on that. Main downside of spy is that it is global, so you have to mark the objects you are interested in somehow, but besides that I think it can be used for redo / undo, as the set of possible mutations is pretty limited and they have all the info needed for undo / redo.

Op za 9 jul. 2016 om 16:06 schreef Aria Fallah notifications@github.com:

Yeah so I should probably document this better, but this is probably the biggest flaw with the library right now.

If you look at this issue mobxjs/mobx#214 https://github.com/mobxjs/mobx/issues/214, MobX currently has no way to do deep observation, and the only solution I can think of would make this library horribly complex and bug prone.

This mean that if you make any changes deeper than one level into the store, there isn't a way to undo them. The only workaround as of now is something like this:

const todos = store('todos')todos.set('id123', { ...todos.get('id123'), 'title: test update todo title' })

Basically, you have to take a redux like approach, and make changes in an immutable manner. This way you're changing the top level id123, and it'll be registered in the history.

The MobX roadmap has a time traveling mechanism listed, but for now this is the best I can do with the current undo/redo implementation.

Is that helpful?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/AriaFallah/mobx-store/issues/34#issuecomment-231535990, or mute the thread https://github.com/notifications/unsubscribe/ABvGhM3IJELcxi13Ex-ecrCJ6sLGNGflks5qT6ragaJpZM4JIjzi .

AriaFallah commented 8 years ago

@mweststrate

Really good idea actually...I can look into this for the time being as well.

mochacoder commented 8 years ago

Thanks for the quick response @AriaFallah that was helpful. @mweststrate yeah the spy API might help me out as well, didn't know it existed.

AriaFallah commented 8 years ago

@mochacoder

Deep observation is now a thing thanks to @mweststrate and his suggestion to use spy! Probably has some quirks, but for the time being it works 😄

You can see how I did it here: https://github.com/AriaFallah/mobx-store/pull/35

You can try it in v3.3.0, and please let me know if there are any issues.

vojto commented 8 years ago

So this still doesn't fire schedule, or am I doing something wrong?

Trying to update local storage when data changes.

AriaFallah commented 8 years ago

@vojto

Could you post the code you're trying to schedule? There are some quirks with when your tasks run so it'd be helpful to have the code.

The scheduling is based on autorun, which I go into a bit of depth about here: https://github.com/mobxjs/mobx/issues/248#issuecomment-218927070. Basically it's not uncommon that in certain cases an autorun will never fire.