-
-
Notifications
You must be signed in to change notification settings - Fork 359
Add Redux/Thunk with bare minimum setup #166
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, a general question. Since we said that we're going to be using hooks in this project, wouldn't it be better to use useSelector
or useDispatch
instead of HOC with mapState/DispatchToProps
https://react-redux.js.org/next/api/hooks
@Zeko369 Using hooks in react needs to be done carefully, and I am no master of it yet. I have come across various tweets/articles mentioning bottlenecks of hooks (which can be resolved, obviously, but with extra care). Check this repo - https://github.com/ryardley/hooks-perf-issues I am not against of using hooks, but I would prefer using |
@Shub1427 I think that we should discuss |
A couple notes: on #123 we resolved to primarily use hooks. We've also agreed to use Thunk. |
Meeting Notes with more on the decision to use hooks. |
Thanks all. Will update my PR accordingly. To keep this in context, this PR is just a setup and does not include, data persistence, neither will it include any SSR support(I was having trouble setting it up with nextjs, but will figure it out). Will open a separate PRs supporting each/both of them. |
f1e2e6d
to
0f518c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't commented on the data fetching since the API for this will be done in a few days so we can work with that, but I had some questions about immer and why not just use overrides on an object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last thing was a comment by accident
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few little changes, also I didn't write it down since I still haven't finished the style guide, but could you add a new line between types of import (npm libs, external modules we creates, local modules)
@Shub1427 In the review I criticized your code, here I'm going to say that this PR is awesome 🚀 I really like the setup and cant wait to start working with it. 🤓 |
Are you working on some Contributing Guidlines on Coding? GREAT!!
I never took them as criticism, but a carefully done assesment, helping me out to do better.
Well I was thinking to skip that and live my life happily ever after. 😜 😆 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general question and pls fix conflicts on package.json
* fix linting errors
* fix: import call sequence
* fix: review changes
Update README.md
).master
branch of Chapter.Details on PR
Closes #145
This PR adds a bare minimum setup for redux/thunk for Chapter app. Fetch calls made is the PR is just for demo purpose and will be replaced with proper modals, once either a mock server is integrated or the api server is merged to the repo.
fetch
proxy lib, in server/browser, with different packages(which are actively maintained).TODO:
redux-persist
for data persistence.redux-persist
, Problem with SSR on v5 rt2zz/redux-persist#457