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

Replace redux code by context API #79

Merged
merged 31 commits into from
Jul 13, 2022
Merged

Replace redux code by context API #79

merged 31 commits into from
Jul 13, 2022

Conversation

swh00tw
Copy link
Member

@swh00tw swh00tw commented Jul 2, 2022

Run e2e tests manually ( feel free to add any item which should be checked ):
https://www.notion.so/jchiroto/E2E-Test-RunBook-16001fb1d7cc4674b2e3b81e3ae24719

  • Move all Redux global states to Context Providers ( CourseSearchingPorvider, CourseTableProvider, DisplayTagsProvider, UserProvider )
  • Move API fetchers under /queries files, and move global states setters out of those fetchers for better reusability and better compatibility if we want to adopt frameworks like SWR.
-------------------- BEFORE --------------------
async function fetcher(){
    const { data } = axios.get(...)
    setState(data)
    return data
}

// in component
useEffect(()=>{
    fetcher();
})
-------------------- AFTER --------------------
async function fetcher(){                      // ✅ better reusability because it did not involve any set states inside, 
    const { data } = axios.get(...)            // and easier to work with frameworks like `SWR`
    return data
}

// in component
useEffect(()=>{
    const data = await fetcher();
    setState(data);
})
  • Also remove try...catch... block in fetchers:

    • because we already wrap the function in try...catch... block outside, so I found it redundant for adding another try...catch... inside.
    • neater code.
    • if we use useSWR hook in future, it already has stuffs like isError & onError callback in syntax like this
    const { data, error } = useSWR(key, fetcher, {
        onSuccess: ()=>{},
        onError: ()=>{},
        ...
    })
    
  • For Hover Course Feature, I found using Context API can't fully replace Redux "performance-wisely" because if there are any states changes in Context Provider, it will force re-render to whole sub-tree and the client become so laggy (discussion 1, discussion 2) (Redux doesn't because it utilize useSelector and it will only re-render component using this hook). So I think if the set states operation happen frequently and the computation is expensive like our hover feature, it's not good to replace redux by context API. So I found a light-weighted library called Valtio (article), it also provide render-optimized feature like redux, and the performance looks good.

    截圖 2022-07-03 下午6 04 08
  • Also unify global states' name to camelCase instead of snake_naming_case

@github-actions
Copy link

github-actions bot commented Jul 2, 2022

Azure Static Web Apps: Your stage site is ready! Visit it here: https://gray-river-017c6bf1e-79.westus2.azurestaticapps.net

Copy link
Member

@jc-hiroto jc-hiroto left a comment

Choose a reason for hiding this comment

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

I've gone through the codes and I will test it tmr.
Nice work! Don't forget to get enough sleep 😪

Comment on lines +66 to +78
<CourseSearchingProvider>
<CourseTableProvider>
<UserDataProvider>
<DisplayTagsProvider>
<Box w="100vw" h={{ base: "100%", lg: "" }}>
<HeaderBar useColorModeValue={useColorModeValue} />
{content(props.route)}
<Footer />
</Box>
</DisplayTagsProvider>
</UserDataProvider>
</CourseTableProvider>
</CourseSearchingProvider>
Copy link
Member

Choose a reason for hiding this comment

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

Since we've gotten rid of redux, there will be more context providers in the future.
To prevent pyramid of doom, I suggest we can solve this based on one of the solutions here.

but ignore this comment, LOL

This is what Redux solves. – coreyward Jul 24, 2018 at 17:33

Copy link
Member

@jc-hiroto jc-hiroto left a comment

Choose a reason for hiding this comment

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

tested without issues.

@swh00tw swh00tw merged commit 594c797 into master Jul 13, 2022
@swh00tw swh00tw deleted the remove-redux branch July 13, 2022 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants