feat: Added defaults into the the flag hooks

[benjackwhite]

Changes

Maybe fixes #714

Need to check for whether this affects hydration of Next.js and what a work around could be...

Checklist

• Tests for new code (see advice on the tests we use)
• Accounted for the impact of any changes across different browsers

[github-actions]

Size Change: 0 B

Total Size: 661 kB

ℹ️ View Unchanged

Filename	Size
dist/array.full.js	171 kB
dist/array.js	113 kB
dist/es.js	113 kB
dist/module.js	113 kB
dist/recorder-v2.js	93.6 kB
dist/recorder.js	58.3 kB

_{compressed-size-action}

[neilkakkar]

makes sense! - not sure about nextjs either

[klaasman]

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 client.isFeatureEnabled(flag) will for example set the initial state to true, where the server-side rendered state was false === hydration error when there's dynamically rendered components based on that flag.

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:

1. user lands on page /foo
2. hook useFeatureEnabled('test') is called
3. hook looks up test flag value in context (useContext(PostHogContext).enabledFlags[flag]) or whatever)
4. state is undefined, fetch state with client.isFeatureEnabled('test')
5. write state to context
6. hook is re-rendered because of updated context
7. useFeatureEnabled returns the value from the context

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;

1. user navigates to /bar
2. user navigates back to /foo
3. hook useFeatureEnabled('test') is called
4. hook looks up test flag value in context (useContext(PostHogContext).enabledFlags[flag]) or whatever)
5. state is found, return the state from context
6. (optionally fetch flag again to update the context value, if that makes sense from a posthog's point of view)

[benjackwhite]

I see you're point but I don't think context is the solution here.

posthog-js caches the flag results both in memory and to localstorage meaning that it basically behaves as a context (i.e. a shared state) across mounts and unmounts.

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

[benjackwhite]

Bigger challenge here is that when posthog-js runs server side, it doesn't work... This means when the code runs like posthog.getFeatureFlag() we get a bunch of errors related to missing properties, especially if the snippet isn't initialised (which we recommend in Next.js docs).

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...

[klaasman]

meaning that it basically behaves as a context

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:

hydrate expects the rendered content to be identical with the server-rendered content. React can patch up differences in text content, but you should treat mismatches as bugs and fix them.

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 useFeatureEnabled('test') yields undefined, and during client-side hydration the useFeatureEnabled('test') yields true, it will result in hydration errors if the final React tree is different based on that flag.

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 localStorage but this is not the same as context, as it is not part of the react lifecycle. The localStorage can be used to populate the context after the first render, and then all should be good.

---

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 renderToString().

[benjackwhite]

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

useFeatureFlagEnabled() // Returns with the result immediately if possible
useFeatureFlagEnabledSSR() // Returns with the result as a second render via state

Feels yuck but I can't find any solid examples out there for hooks with cached content stored in localstorage...

[benjackwhite]

Seems like this lib solved it with a custom hook astoilkov/use-local-storage-state#23

[benjackwhite]

Ballooned the PR but possibly for the better. Essentially this allows you to specify the ssr: false option to avoid the initial useEffect call as well as using LayoutEffect which should generally reduce flicker.

This doesn't solve the linked issue as moving around the app will still mean more waiting for the useEffect handler.

I gave it a shot but for the life of me could not get it working, whether with a global variable or with context. Next.js docs feel a little useless here tbh and it hit the diminishing returns point. @klaasman if you can get a working PR then I'd be happy to pull it in but at least locally I couldn't get a single solution to work. As soon as I navigated to another page, the context was lost and it started again.

I think it works now. Global var to track hydration seems to work great and is super low touch

[Twixes]

What does it change that this is now a useLayoutEffect in the browser?

[klaasman]

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:

1. render
2. paint browser
3. run effect
4. mutate state
5. re-render
6. paint browser

a useLayoutEffect calling setState() looks like:

1. render
2. run layoutEffect
3. mutate state
4. re-render
5. paint browser

Downside is that useLayoutEffect can have negative impact on render performance since it's blocking whereas useEffect is asynchronous.

[Twixes]

Oh, I see, thanks!

[Twixes]

Code looks good to me, although I've not ran it. Are the playground changes covered by tests?

[Twixes]

Oh, I see, thanks!

[posthog-bot]

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 stale label – otherwise this will be closed in another week.

[posthog-bot]

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 stale label – otherwise this will be closed in another week.

[posthog-bot]

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.