-
Notifications
You must be signed in to change notification settings - Fork 97
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
feat: Added defaults into the the flag hooks #717
Conversation
Size Change: 0 B Total Size: 661 kB ℹ️ View Unchanged
|
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.
makes sense! - not sure about nextjs either
This solution will most likely result in react hydration errors (= not per se next.js only) if the client is loaded (including fetching feature flags) before the component gets hydrated. In those cases the I don't think there's a safe way to have the initial state set w/o hydration errors. But, it is possible to have subsequential renders (e.g. client-side page navigation foward, backward) use the already present flag value as initial value, but it must be "known" to the react lifecycle. This can, I think, be accomplished by writing the feature flag states to a context. e.g. first render cycle:
With this first render there will initially be no value returned from the hook. Then, after the flag is fetched, it will rerender with the desired result. Not so much to do about this and simply a part/downside of the client-side flag fetching. then;
|
I see you're point but I don't think context is the solution here.
The only environment we need to solve is Next.js that is very adverse to having a first render that doesn't match the server rendered version. Still need to check what can be done here but the benefit of having the correct flag value as soon as possible I think outweighs the other issues, so long as we can suppress the Next.js or offer a workaround for that specific environment |
Bigger challenge here is that when posthog-js runs server side, it doesn't work... This means when the code runs like Need to try a couple of things and figure out a good approach here. We could use a dedicated React context but I feel like we're just moving the problem instead of solving it in the JS properly... |
I don't think this is true but please let me know if I miss your point. According to https://react.dev/reference/react-dom/hydrate#caveats:
If the client-side hydrate is called with different data resulting in a different React tree, it will be seen as an "hydration error". When server-side the They also have a section on Handling different client and server content where they explain to do a two-pass render. The first render (a.k.a. hydration) should be identical to the server-side render. Then state (or context!) can be mutated in order to update the application to its final React tree. It's perfectly fine to have state in Note: all the above should be true for any framework/library which supports server-side rendering + client side hydration. Even a custom Express server with |
Not sure what the solution is here tbh. If we optimise for the best experience we end up breaking the rules of hydration. If we optimise for server side rendering then we end up with double rendering when it is entirely unnecessary. The middle ground of storing some shared state across hooks might be the best scenario but it feels hard to explain and somewhat error prone... Maybe we need two methods - one for those clients who don't need to worry about hydration and one for those that do like
Feels yuck but I can't find any solid examples out there for hooks with cached content stored in localstorage... |
Seems like this lib solved it with a custom hook astoilkov/use-local-storage-state#23 |
Ballooned the PR but possibly for the better. Essentially this allows you to specify the This doesn't solve the linked issue as moving around the app will still mean more waiting for the useEffect handler.
I think it works now. Global var to track hydration seems to work great and is super low touch |
@@ -8,7 +9,7 @@ export function useActiveFeatureFlags(): string[] | undefined { | |||
// would be nice to have a default value above however it's not possible due | |||
// to a hydration error when using nextjs | |||
|
|||
useEffect(() => { |
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.
What does it change that this is now a useLayoutEffect
in the browser?
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 can prevent a "flickering" ui where an initial render is not displaying an element, and a second render caused by setState
does display an element.
a useEffect
calling setState()
looks like:
- render
- paint browser
- run effect
- mutate state
- re-render
- paint browser
a useLayoutEffect
calling setState()
looks like:
- render
- run layoutEffect
- mutate state
- re-render
- paint browser
Downside is that useLayoutEffect can have negative impact on render performance since it's blocking whereas useEffect is asynchronous.
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.
Oh, I see, thanks!
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.
Code looks good to me, although I've not ran it. Are the playground changes covered by tests?
@@ -8,7 +9,7 @@ export function useActiveFeatureFlags(): string[] | undefined { | |||
// would be nice to have a default value above however it's not possible due | |||
// to a hydration error when using nextjs | |||
|
|||
useEffect(() => { |
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.
Oh, I see, thanks!
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
Changes
Maybe fixes #714
Need to check for whether this affects hydration of Next.js and what a work around could be...
Checklist