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

[BUG]: useMemo Being a rouge agent on useHybridHydrate #540

Open
wants to merge 1 commit into
base: 8.x
Choose a base branch
from

Conversation

adenekan41
Copy link

What happened ?

While attempting to utilize the wrapper store within the getServerSideProps function provided by the wrapper, an error was encountered. Specifically, this error was initially observed when transitioning from statically generated pages (SSG) to server-side rendered (SSR) pages. However, the issue was further compounded when attempting to revisit the same SSR link from the navigation bar, causing the error to appear twice consecutively.

Screenshot 2023-04-17 at 3 05 48 AM

What did i change ?

I have implemented a fundamental change in how we handle hydration updates. Although the code was already impressive, the warning message made it clear that a setState call was being triggered within the render method of the App component while a different component (PlaylistView) was being rendered. This action is prohibited in React as it results in unpredictable behavior and may cause the application to crash or malfunction.

To tackle the issue I update the check mechanism. By allowing React to complete its lifecycle, proper data handling is ensured, resulting in an overall improvement in the fluidity of the data hydration process.

How can I reproduce this error?

  • Run a dispatch from your getServerSideProps that in turn updates the HYDRATE state
  • Try to navigate fluidly on your application from CSR -> SSG -> SSR and this bug should be reproduced

Reviewers Note

This entire issue is caused by an early useMemo what we want to do is hydrate when you are unMounting and rehydrate when you are Mounting "easy peezy"

…omplete functions outside of useEffect hook to avoid creating new function references on each render

feat(wrapper): add memoizedHydrateOrchestrator function to ensure that the function is only created when any of the dependencies passed in the second argument to useMemo changes.
@ernestchakhoyan
Copy link

LGTM! @kirill-konshin when you will be able to merge this pr and deploy new version? It is very important and useful PR

@dovranJorayev
Copy link

Tried to apply fix with patch-package and it is still there

@dovranJorayev
Copy link

dovranJorayev commented Nov 20, 2023

Tried to apply fix with patch-package and it is still there

Was wrong about the fix. It is do the job.
I have cloned fixed fork, builded it. After patch are applied no issues are here (Also need to remove cache). Thanks a lot @adenekan41

@kaminion
Copy link

Tried to apply fix with patch-package and it is still there

Was wrong about the fix. It is do the job. I have cloned fixed fork, builded it. After patch are applied no issues are here (Also need to remove cache). Thanks a lot @adenekan41

How did you build? when I use yarn, that's occur typescript error. it's not solved

@dovranJorayev
Copy link

dovranJorayev commented Jun 12, 2024

I didn't really remember, but i didn't do any special. I just forked, installed with same version yarn that has in forked repo, build, apply the patch and it's done. I can share the gist of generated *.patch if it will be helpfull. Generally it is all needed to place it to {project}/patches/{patch-name})

@kaminion
Copy link

kaminion commented Jun 12, 2024

I didn't really remember, but i didn't do any special. I just forked, installed with same version yarn that has in forked repo, build, apply the patch and it's done. I can share the gist of generated *.patch if it will be helpfull. Generally it is all needed to place it to {project}/patches/{patch-name})

thanks,
I just solve the problem!, set the node version and yarn version remove the cache in the directory

@jwickens
Copy link

jwickens commented Oct 8, 2024

Has anyone found that this warning causes any actual issues beyond the warning?

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.

5 participants