#31 redux-toolkit reducer configuration
Opened by divyanshisingh. Modified
fedora-qa/ divyanshisingh/landingpage redux-toolkit-reducer-config  into  master

Download 31.patch

This PR is linked with #29
1) Implemented sliced reducer using redux-toolkit.
2) Changed redux actions to use actions created by redux-toolkit.
3) Implemented actions to support asynchronous calls.

@lbrabec I have closed #30 as its commit is included in this. Please, review it and let me know if some changes are required.

Do we need this at all?

Parameter initialState (or preloadedState as it is called in redux toolkit docs) for configureStore() seems to be optional.

Maintaining store structure manually is going to be prone to errors. It is basically duplication of code in wizard slice and landingpage slice.

I think spread operator would look better:

state = {
    loading: false,
    ...action.payload
}

Ok, @lbrabec I will make the required changes soon.
Thank you for your review and feedback.

In browser console I see action undefined:

action undefined @ 14:10:15.918
redux-logger.js:1  prev state {landing_page: {…}, wizard: {…}}
redux-logger.js:1  action     ƒ (dispatch) {…
redux-logger.js:1  next state {landing_page: {…}, wizard: {…}}

I presume it is the dispatch of fetchLandingPageData().

Is it possible to give a name to this action, so the logs don't say action undefined but something like action fetchLandingPageData?

Metadata Update from @lbrabec:
- Pull-request tagged with: review in progress

1 new commit added

  • removed undefined actions in logger

The Reducer is split so that we can keep the slices in their respective components along with the actions as done in wizardSlice and landingPageSlice, I thought this would be useful if our codebase becomes too large. So, I didn't understand how there will be duplicate code since the index.js in the reducer folder will no longer be required. Should we combine the reducers and use them as before?

The spread operator as mentioned by you will not work in the redux-toolkit. More descriptions of it can be found in this link https://stackoverflow.com/questions/60002846/how-can-you-replace-entire-state-in-redux-toolkit-reducer

The action logged as "undefined" is handled in the new commit.

The Reducer is split so that we can keep the slices in their respective components along with the actions as done in wizardSlice and landingPageSlice, I thought this would be useful if our codebase becomes too large. So, I didn't understand how there will be duplicate code since the index.js in the reducer folder will no longer be required. Should we combine the reducers and use them as before?

No, the split is definitely the right direction. What I meant is that now we have partial reducers and partial states in each slice, which are then combined to form root reducer and "root state". That is indeed a good separation of concerns and will help us in the long run.

But you added export to const defaultState in src/reducers/index.js and use it const store= configureAppStore(defaultState) in src/index.js. The defaultState is duplication of code present in each slice, if you change partial state in slice, you would have to change manually the defaultState as well. If I read the documentation correctly, you don't have to pass any default/initial/preloaded state to configureStore() in src/configureStore.js. So I propose to get rid of const defaultState altogether.

The spread operator as mentioned by you will not work in the redux-toolkit. More descriptions of it can be found in this link https://stackoverflow.com/questions/60002846/how-can-you-replace-entire-state-in-redux-toolkit-reducer

Oh, ok.

The action logged as "undefined" is handled in the new commit.

Great! I just noticed that there is a new file package-lock.json. I guess it is there because you used npm instead of yarn. Please delete that file.

1 new commit added

  • removed defaultState from store configuration

I have removed the defaultState from configureStore. Please review.

Great! Now you can completely remove defeaultState from src/reducers/index.js. Since it is not used anymore, it really should be in code as it could cause confusion later.

Once you removed the defaultState, please squash all the commits to one and this PR will be done :)

rebased onto 8388eaf94d23cd21ce9f3f6eeda3c49c2dbfbab9

I have squashed the commits and also removed src/reducers/index.js as it's no longer required.

Thanks!

Metadata Update from @lbrabec:
- Pull-request untagged with: review in progress
- Pull-request tagged with: review done

Metadata