Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Add Redux/Thunk with bare minimum setup #166

Merged
merged 6 commits into from
Dec 5, 2019

Conversation

phoenisx
Copy link
Contributor

@phoenisx phoenisx commented Nov 12, 2019

  • I have read Chapter's contributing guidelines.
  • My pull request has a descriptive title (not a vague title like Update README.md).
  • My pull request targets the 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.

  • Separates fetch proxy lib, in server/browser, with different packages(which are actively maintained).
  • Using Redux Hooks for store state and dispatching actions from the component itself.

TODO:

  • Add redux-persist for data persistence.
  • Add SSR support to cache and re-hydrate redux state in react app
  • Typescript integration with Redux is still very hacky, any suggestions or later RFCs might resolve them.

Some changes:

  • I have added modules folder to keep page related layout components in them, all the UI specific generic components are put in components dir.
  • Also, I have kept Http Service in a separate layer, so that if needed, we can replace it to whatever API service we want to use. Specific http services (eg, Chapter Service) should be derived or be composed of HttpService.

PS: Also, this is my first ever biggest PR for a community project, if there are improvements needed to be done in the PR, please let me know, I would be happy to improve myself.

Copy link
Member

@Zeko369 Zeko369 left a 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

client/containers/home.tsx Outdated Show resolved Hide resolved
client/hoc/with-store.tsx Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@phoenisx
Copy link
Contributor Author

@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 PureComponent with redux for page modules and functional components (with hooks, if needed) for UI components.

@Zeko369
Copy link
Member

Zeko369 commented Nov 14, 2019

@Shub1427 I think that we should discuss hooks/React.PureComponents/React.Component on the team meeting. @QuincyLarson Could you add this to the agenda?

@QuincyLarson
Copy link
Contributor

A couple notes: on #123 we resolved to primarily use hooks. We've also agreed to use Thunk.

@allella
Copy link
Contributor

allella commented Nov 16, 2019

Meeting Notes with more on the decision to use hooks.
https://github.com/freeCodeCamp/chapter/wiki/Meeting-Notes-November-15,-2019

@phoenisx
Copy link
Contributor Author

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.

@phoenisx phoenisx force-pushed the add-redux branch 2 times, most recently from f1e2e6d to 0f518c7 Compare November 18, 2019 17:57
Copy link
Member

@Zeko369 Zeko369 left a 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

client/store/reducers/chapter.ts Show resolved Hide resolved
client/services/http-service.ts Outdated Show resolved Hide resolved
pages/_app.tsx Outdated Show resolved Hide resolved
@Zeko369 Zeko369 self-requested a review November 23, 2019 00:23
Copy link
Member

@Zeko369 Zeko369 left a 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

package.json Outdated Show resolved Hide resolved
Copy link
Member

@Zeko369 Zeko369 left a 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)

client/interfaces/components/AddSponsor.tsx Outdated Show resolved Hide resolved
client/modules/home.tsx Outdated Show resolved Hide resolved
client/store/types/chapter.ts Show resolved Hide resolved
@Zeko369
Copy link
Member

Zeko369 commented Nov 26, 2019

@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. 🤓
PS. you still need to fix things from the review 😜

@phoenisx
Copy link
Contributor Author

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)

Are you working on some Contributing Guidlines on Coding? GREAT!!
Even I was thinking to do so after this PR. But if you are doing it, thanks.
Please open an issue regarding that so everybody can also contribute their thoughts.

@Shub1427 In the review I criticized your code, here I'm going to say that this PR is awesome rocket I really like the setup and cant wait to start working with it. 🤓

I never took them as criticism, but a carefully done assesment, helping me out to do better.
Thanks for that @Zeko369.

PS. you still need to fix things from the review 😜

Well I was thinking to skip that and live my life happily ever after. 😜 😆

Copy link
Member

@Zeko369 Zeko369 left a 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

client/store/types/chapter.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Zeko369 Zeko369 merged commit b8fab09 into freeCodeCamp:master Dec 5, 2019
@phoenisx phoenisx deleted the add-redux branch December 5, 2019 09:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide which state management tool to use
6 participants