mheiber / redux-machine

A tiny library (12 lines) for creating state machines in Redux apps
336 stars 16 forks source link

createMachine might do an unnecessory copy of a returned state #9

Closed zhoudxnz closed 7 years ago

zhoudxnz commented 7 years ago

I noticed when the redux state wasn't changed after an action and my reducer returned the original state, the machine returned a deep-copied state.
It would be more efficient if the machine returns this: return Object.assign({}, nextState, {'status': nextState.status || status}) ==> return (nextState.status) ? nextState : {...nextState, 'status' : status};

mheiber commented 7 years ago

@zhoudxnz copies with Object.assign and {... } are shallow, so I'd be surprised if there were a performance problem here. Are you running into any problems?

Also (just out of curiosity), what's your use case for unhandled actions?

Thanks for trying redux-machine!

zhoudxnz commented 7 years ago

I noticed that when one of my test cases failed after using the state machine. I didn't expect the state to be changed by the reducer and used "===" to assert that. I am writing a simple board game to learn react/redux. By the game logic, if a player clicks on a selected ball again, the move is invalid and the action shouldn't trigger a state change and UI refresh. It might not cause a measurable performance problem to my case, but as a programmer, I just felt itchy when I realized copy in that case is unnecessary even it is a shallow one. Anyway, thanks for your work. Btw, I think it would be good if createMachine could take an extra argument for the name of initial status. So a user can choose a name consistent with naming convention of the project.

mheiber commented 7 years ago

Thanks for sharing your use case. I'll look into returning the same state in the next few days.

Your test will be less brittle if you do a deep equality check. The reason is that you may want to add something like a counter for the number of moves so far, or a message saying when a move is invalid.

About taking an extra argument for the initial status: on the face of it, that's a good idea. However, redux-machine already hides some of the app logic. Having configuration somewhere for the name of the initial state would make it harder to look at app code and know what it will do. It totally makes sense to want to follow the app's naming conventions, but then it may be better to be explicit and not use redux-machine. There's an example in the README of following the same pattern as redux-machine without a library.

mheiber commented 7 years ago

Hope this helps!