mattkrick / redux-optimistic-ui

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

Support intiialState in deep optimistic reducers without preloadState #23

Closed marcins closed 7 years ago

marcins commented 7 years ago

Currently the preloadState function is used to provide intiial state in a form that redux-optimist-ui expects, however by instead also initialising the state on the @@redux/INIT action we can setup the state correctly without explcitly calling preloadState in the state we pass to createStore.

Fixes #22

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 92.308% when pulling fc63d8ecb20f3a9ee0f3c2b2f6b195763c0f924c on marcins:bugfix/support-initial-state-without-preload into 6868145934031b4bbe2530e805520624c4fea375 on mattkrick:master.

mattkrick commented 7 years ago

oh interesting. waaaaaay back when that's what i used instead of isReady but i can't remember why i switched it out. if this gets rid of the need for preload state could you rip that piece out?

marcins commented 7 years ago

Looks like isReady was added in b5052c555c6b0d164ca0d0e6702ea8c47d6e3bb4, replacing using initialState as a default arg for when state was undefined (there's no version using Redux's INIT action that I can see in the public history at least).

Was it related to enhancing a reducer asynchronously later after it already had state? I haven't checked, but maybe that doesn't trigger a an INIT. Will try it out later today.

marcins commented 7 years ago

So in this kind of scenario Redux does dispatch an @@redux/INIT action through the reducers again:

const newReducer = Object.assign({}, rootReducer, { users: optimistic(usersReducer) });
store.replaceReducer(combineReducers(newReducer));

Which means removing the isReady bits entirely and just having this works:

if (state === undefined || action.type === '@@redux/INIT') {
    state = preloadState(reducer(ensureState(state), {}));
}

All the tests still pass with this except the "wraps a reducer with existing state" test - however that test passes the action {} in to produce the actual value to test against. Changing that to pass in { type: '@@redux/INIT' } fixes the test.

My question - is the current test where an empty action is passed to the reducer to get it to produce an initial value realistic in production, or is depending on @@redux/INIT appropriate here?

I'm wary of the fact that my current initial uses are relatively simple, and there's some advanced scenario with async loading / replacing reducers that I'm not considering.

mattkrick commented 7 years ago

dang, wish i wrote a note about why i didn't use the redux init action... i know i did that in https://github.com/mattkrick/redux-operations but i was using a store enhancer & did some pretty crazy stuff.

@clayne11 it looks like this would eliminate the need for a preload state helper. is this something you're interested in/can you verify if it works for your usecase?

mattkrick commented 7 years ago

@marcins sorry i let this one fall through the cracks. @marcins if this eliminates the need for preloadState, could you remove that from the API as well so we can keep this thing nice n tight?

marcins commented 7 years ago

I’ve been on holidays for the last week, I’ll try and have another look at this later this week.

On 22 March 2017 at 05:55, Matt Krick notifications@github.com wrote:

@marcins https://github.com/marcins sorry i let this one fall through the cracks. @marcins https://github.com/marcins if this eliminates the need for preloadState, could you remove that from the API as well so we can keep this thing nice n tight?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mattkrick/redux-optimistic-ui/pull/23#issuecomment-288182916, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUx2Qpiljxh5E9WjpR9ZqZgPVWI3bpEks5roB05gaJpZM4MKw7B .

-- M.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 92.308% when pulling 83cbc608f39cbc6faee1fc229d5435bb04529c39 on marcins:bugfix/support-initial-state-without-preload into 6868145934031b4bbe2530e805520624c4fea375 on mattkrick:master.