search

mattkrick / redux-optimistic-ui

a reducer enhancer to enable type-agnostic optimistic updates
MIT License
693 stars 36 forks source link

optimistic.id is removed on commit and revert #9

Closed m-lautenbach closed 8 years ago

m-lautenbach commented 8 years ago

I noticed that you set optimistic.id to undefined on REVERT or COMMIT. We would like to include that id in our unittests, to make sure it is set properly. It's also very irritating in the dev console. Can this be avoided?

mattkrick commented 8 years ago

anything is possible, however i'd _strongly_ suggest you reevaluate your unit test methodology. unit tests should be testing for a correct result not how that result is achieved. if you're testing how something works instead of whether it works or not, you're probably doing it wrong :smile:

m-lautenbach commented 8 years ago

We would like to test the actions our legacy code dispatches without testing the reducers. So the dispatched actions would be the result of the unit. Right?

mattkrick commented 8 years ago

I'm not exactly sure I understand your process correctly, but I'd write 2 different test types. 1 for action creators, and another for the reducer. I'd even argue that the action creator type is redundant since the code will be covered when writing a reducer test. http://redux.js.org/docs/recipes/WritingTests.html

I'll close for now since I think this is operating as designed, but please feel free to keep the dialog going, as I've definitely been wrong in the past!

m-lautenbach commented 8 years ago

Under the link, chapter 'Async Action Creators', it's recommended to mock the store and test on the dispatched actions. That's what we do and why the removed transaction id is a problem for us. At some point this code will be sagas, but that's still quite a way and we need tests now as well.

ChristophVitz-ItSonix commented 8 years ago

I just stumbled over the same issue, but not because of unit-testing. I realised that the transactionId property of a dispatched action is modified after it was dispatched. This seems a bit odd, as actions should be considered immutable and all functions that process these actions (such as reducers) should be pure and not modify them. This is especially confusing when debugging using the dev tools. I thought that our application code was pushing optimistic actions with an incorrect id and searched for a bug in our code - until I found out that this is caused by the optimistic-ui itself. Is there a better, more transparent way to do this? Maybe adding a ignore flag or something like that?

mattkrick commented 8 years ago

@ChristophVitz-Actano Your actions are yours, and they are immutable: https://github.com/mattkrick/redux-optimistic-ui/blob/master/src/index.js#L48-L51. If you want to store them for troubleshooting feel free to do so, but again, I'd caution that you should treat 3rd party packages as a black box (although it's great to understand what's inside!). Test the inputs, test the outputs, don't worry about how those results are achieved, because in the future, the how may change.

See how it's done in the tests: https://github.com/mattkrick/redux-optimistic-ui/blob/master/__tests__/index-tests.js.

ChristophVitz-ItSonix commented 8 years ago

I was referring to this line https://github.com/mattkrick/redux-optimistic-ui/blob/master/src/index.js#L126. The actions are modified in this place and the redux-devtools actually shows the modified actions (with an explicit transactionId property that has the value undefiend).

This also causes buggy behaviour when using the devtools to disable and then re-enable a reverting operation. In this case the revert is not applied anymore and the original (optimistic) change is applied to the store data.