dzcode-io / dzcode.io

Website & mobile app for Algerian open-source community
https://dzcode.io
MIT License
114 stars 41 forks source link

Chore: Using `@reduxjs/toolkit` in mobile codebase #481

Closed omdxp closed 1 year ago

omdxp commented 1 year ago

Description

This PR is dedicated to use Redux Toolkit in favor of Redux. It'll help with DX as well as eliminating some current performance issues.

Resolves #482 Fixes #375

Type of change

ZibanPirate commented 1 year ago

You really wanted this change haha

I was thinking of doing something like:

https://github.com/ZibanPirate/trove/tree/main/src/redux

Where we have slices in one file, in the same dir as store configuration as well as our homemade subscription function.

And we have action creators in different dir (thunks) where they can be tree shaked and fetched separately from main bundle in the chunk of whatever page required it.

There are some black magic typing there, but it's essentially just helping mapping the slices into one state interface, that can also be used for our own useSliceSelector

I may have gone overboard with my own approach, so I would like to see how you gonna approach this, and have a collaborative approach

omdxp commented 1 year ago

You really wanted this change haha

I was thinking of doing something like:

https://github.com/ZibanPirate/trove/tree/main/src/redux

Where we have slices in one file, in the same dir as store configuration as well as our homemade subscription function.

And we have action creators in different dir (thunks) where they can be tree shaked and fetched separately from main bundle in the chunk of whatever page required it.

There are some black magic typing there, but it's essentially just helping mapping the slices into one state interface, that can also be used for our own useSliceSelector

I may have gone overboard with my own approach, so I would like to see how you gonna approach this, and have a collaborative approach

I will try to make it as similar as the old codebase, however in this new structure the actions and reducers are merged in slices folder, so each slice folder will contains its specific actions. It's a good idea to use RTK Query in this case, but I'll start with using extra reducers as thunk actions and probably later on we'll switch to RTK Query which I think it's the best approach.

You can find here my approaches in RTK Query: https://github.com/Omar-Belghaouti/blogs-full-stack/tree/main/frontend/src/store https://github.com/Omar-Belghaouti/todo-full-stack/tree/main/frontend/src/store/api

As said above, for this PR I'll work on it without RTK Query however it'll have similar structure. And you can work in the web codebase in the meantime I work in the mobile codebase.

omdxp commented 1 year ago

@ZibanPirate check out the new performance for the contribution filters. Now with RTK we can store array data in a normalized form with the help of entity adapters, I was actually planning to normalize it manually here πŸ˜… Thanks to RTK it's much easier to normalize data which guarantees performance for arrays data.

omdxp commented 1 year ago

I know there are many tasks in this PR or should I say it looks like it πŸ˜…

  • moved src/redux to src/store, which didn't help the diff, and made it a bit harder to spot the changes.

For this I had to rename the redux folder to store so there will no ambiguity when importing stuff when using relative imports like we talked about it here.

  • refactored the state, and introduced new properties like status.

The new status property is more generalized than the old refreshing property. In this version we can definitely know what is happening in the current state (if it's refreshing or not + if there is an error). So you can think of it as it's holding 3 states of the current slice which are idle (refreshing: false), loading (refreshing: true) and error if there was an error when fetching. The reason it was defined as it is was because of the cases in the extraReducers property in the slice, those cases like pending, fulfilled and rejected are defined out of the box for all async thunks which is more suitable in this case.

  • an attempt to "fix" the contributions filters lag.

To be honest I wasn't trying to fix that lag, it was fixed just by implementing the array states as entity adapters that's all. The UI code as you may noticed is not changed at all expect the parts of selecting the data which I thought it's good to have multiple selectors that shows within the code what state are we selecting.

Now, replying to your comments:

  • Let’s make a clear difference between Actions (that we define in the slice object), and Action Creators (aka Async Actions aka Thunks). meaning that the slice shouldn't know about any Thunk, and updating the status should be moved from the slice .addCase(fetchContributions.pending, into the Thunk body, and let the Thunk handle the status changes (which again is out of this PR scope to add status in the first place). and finally move these thunks outside the state files so they could be tree-shaked (not that needed for mobile code, but better have consistency with web).

As said above, those cases are predefined and the status property is more suitable for it. I believe there is a state that isn't playing major factor during production which is error but I thought it'd definitely be helpful during debugging as it holds the error.message if any bug in that situation is occurred. Also, it is recommended to put it this way, I mean to fetch data in the createAsyncThunk and only set the data in extraReducers otherwise we would have the same old getState and dispatch which is not ergonomic especially when we have all the capabilities given from @reduxjs/toolkit.

  • Can we have just one selector? it's not like we have any optimization out of writing a selector for each specific path from the state. It's just too much code I think.

For this I thought that it is more readable to see which state we are selecting within the code (ex: useSelector(selectArticles)). I am not sure if you are saying that we have one selector for the global state or for each one of the slices (that mostly holds the data, status and error). If it's the latter case I will export one selector for each slice so we can have one useSelector statement in the UI.

So I hope this clears things up. And let's see your approach on the web and do some Discussions afterwards.

ZibanPirate commented 1 year ago
  • moved src/redux to src/store, which didn't help the diff, and made it a bit harder to spot the changes.

For this I had to rename the redux folder to store so there will no ambiguity when importing stuff when using relative imports like we talked about it here.

hmm fair, but my only request is to temporarily put the folder name back to src/redux in this PR, and we renamed it in a follow-up PR πŸ™, I tried adding comments but they seemed out of context because of the diff. Once you rename it, I wil put all my comments for all the tasks in this PR πŸ˜„ (and i hope you will have fun addressing them 😈 πŸ˜…)

omdxp commented 1 year ago

Oh I will πŸ˜…, I'll rename it back