Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor collections reducers and actions using redux toolkit #3125

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

PiyushChandra17
Copy link
Contributor

Associated issue: #2042

Changes:

  • Removed all the associated constants
  • Used createSlice to rewrite the collections reducers
  • Exported the automatically generated action creator functions

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@raclim
Copy link
Collaborator

raclim commented Jun 13, 2024

Thanks for your work on this! I'm testing this out by running a branch and I think all the functionality generally works except for the one for deleting a collection. Would you be able to update this PR so its functionality mirrors the one that's currently up?

@PiyushChandra17
Copy link
Contributor Author

Thanks for your work on this! I'm testing this out by running a branch and I think all the functionality generally works except for the one for deleting a collection. Would you be able to update this PR so its functionality mirrors the one that's currently up?

@raclim Sure!

@PiyushChandra17 PiyushChandra17 force-pushed the piyush/refactor-collections-reducers-actions-redux-toolkit branch from 66186f1 to d11ed16 Compare June 13, 2024 19:36
@PiyushChandra17
Copy link
Contributor Author

@raclim Updated this PR to delete a collection, can you please verify once if it's working? Thanks!

Copy link
Collaborator

@lindapaiste lindapaiste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine I guess. It's no worse than what we have now but I do think that there are other "tools" in the Redux Toolkit that can be more helpful, such as createAsyncThunk and RTK Query. I don't know how much refactoring @raclim is comfortable with, but it seems like we're side-stepping the actual pain points here. The reducer was and still is very simple, it's the thunk actions that are messy.

We can use createAsyncThunk where we define the main asynchronous action (the API call) and it automatically dispatches "loading" "success" and "error" actions for it. Our existing loading reducer only looks for actions with the exact type of startLoader and stopLoader, but we could update it to look for any of the async thunk action types using matchers.

We still would not know what is loading. This has caused issues before such as #2250 and I've had to put in work-arounds #2996 (comment). That is where RTK Query can help. We would define an "endpoint" for each of these actions. It automatically manages the Redux state, with a separate status for every API call. In the components we would write something like const { data, isError, isFetching } = useGetCollectionsQuery(username).

client/modules/IDE/actions/collections.js Outdated Show resolved Hide resolved
@PiyushChandra17
Copy link
Contributor Author

This is fine I guess. It's no worse than what we have now but I do think that there are other "tools" in the Redux Toolkit that can be more helpful, such as createAsyncThunk and RTK Query. I don't know how much refactoring @raclim is comfortable with, but it seems like we're side-stepping the actual pain points here. The reducer was and still is very simple, it's the thunk actions that are messy.

We can use createAsyncThunk where we define the main asynchronous action (the API call) and it automatically dispatches "loading" "success" and "error" actions for it. Our existing loading reducer only looks for actions with the exact type of startLoader and stopLoader, but we could update it to look for any of the async thunk action types using matchers.

We still would not know what is loading. This has caused issues before such as #2250 and I've had to put in work-arounds #2996 (comment). That is where RTK Query can help. We would define an "endpoint" for each of these actions. It automatically manages the Redux state, with a separate status for every API call. In the components we would write something like const { data, isError, isFetching } = useGetCollectionsQuery(username).

@lindapaiste Exactly! we can create one apiSlice file and use it in apiSlice.injectEndpoints for every actions. Yes we need to follow this convention useGetCollectionsQuery and if it's mutation then instead of query, it would be mutation and we use these in the components. As far as isLoading, isError, isFetching is concerned we can destructure them and use it right out of the box on a fly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants