PostHog / posthog-js

Send usage data from your web app or site to PostHog, with autocapture.
https://posthog.com/docs/libraries/js
Other
303 stars 126 forks source link

[react hooks] Unnecessary re-render when feature flag is already loaded #714

Open klaasman opened 1 year ago

klaasman commented 1 year ago

It seems that all react hooks initially render w/o the feature flag value and they will be populated after the internal useEffect has ran. See for example the following hook coming from useFeatureFlagEnabled.ts:

export function useFeatureFlagEnabled(flag: string): boolean | undefined {
    const client = usePostHog()

    const [featureEnabled, setFeatureEnabled] = useState<boolean | undefined>()
    // would be nice to have a default value above however it's not possible due
    // to a hydration error when using nextjs

    useEffect(() => {
        return client.onFeatureFlags(() => {
            setFeatureEnabled(client.isFeatureEnabled(flag))
        })
    }, [client, flag])

    return featureEnabled
}

Each and every time this hook is called, the initial (default) featureEnabled will be undefined and after the useEffect has ran it will be set to false or true.

According to the comment in the above function, someone has already thought about having the "default" value in the initial state but ran into hydration issues. This is true, but in theory it should be possible to write the flag state to a context and read values from there on subsequential renders.

Are there internal posthog limitations for why this wouldn't be possible?

Some background info: I'm running into a use-case with a next-app where a user lands on page /foo. That page will load feature flag flag-x in order to show something. Then he navigates (client-side) to /bar. That page also depends on flag-x to show something. It is expected for the first page (/foo) to shortly toggle between hide and show of a piece of content because of PH loading the flags. But the second page should be able to render instantly with a hidden state, because it is rendered client-side and flags have already been loaded.

KutnerUri commented 6 months ago

what the heck? I'm guessing it's a problem if the server renders one thing, and the client renders another, but that's just the initial render! there's no reason to drag this across the app's entire lifecycle.

The Provider can do this more efficiently, ie.

client.haveFeatureFlags
client.isInitialRender / isHydrationRender
wshart commented 2 months ago

This is quite a frustrating gotcha, but easily solved with a custom hook:

export function useFeatureFlagEnabledWithCurrentValueAsDefault(flag: string): boolean | undefined {
    const client = usePostHog()
    const defaultFlagValue = client.isFeatureEnabled(flag) // this just locally retrieves the current value...

    const [featureEnabled, setFeatureEnabled] = useState(defaultFlagValue) // ...that we can use as the default for our hook

    useEffect(() => {
        return client.onFeatureFlags(() => {
            setFeatureEnabled(client.isFeatureEnabled(flag))
        })
    }, [client, flag])

    return featureEnabled
}

It would be nice if this could be included in the official library 🙂

Edit: It would still be nice to have a workaround for nextjs

klaasman commented 2 months ago

That custom hook suggestion can result in hydration errors, basically what they concluded themselves: https://github.com/PostHog/posthog-js/blob/26150b197dc1a2f5241b8d29ee7a359331a1a0a4/react/src/hooks/useFeatureFlagEnabled.ts#L8-L9

wshart commented 2 months ago

Not if you aren't using nextjs 🙂

klaasman commented 2 months ago

Ah right, yeah it should be ok if it's only client-side we're talking about.

DogeAmazed commented 1 month ago

That custom hook suggestion can result in hydration errors, basically what they concluded themselves:

https://github.com/PostHog/posthog-js/blob/26150b197dc1a2f5241b8d29ee7a359331a1a0a4/react/src/hooks/useFeatureFlagEnabled.ts#L8-L9

Though they could check whether or not feature flags were bootstraped (https://posthog.com/tutorials/nextjs-bootstrap-flags).

export function useBootstrapedFeatureFlagVariantKey(flag: string): string | boolean | undefined {
    const client = usePostHog()

    // Check if the feature flag is already bootstrapped. If so, use that value for (atleast) the first render
    const bootstrapedFeatureFlag = client.config.bootstrap?.featureFlags ? client.getFeatureFlag(flag) : undefined;
    const [featureFlagVariantKey, setFeatureFlagVariantKey] = useState<string | boolean | undefined>(bootstrapedFeatureFlag)

    useEffect(() => {
        return client.onFeatureFlags(() => {
            setFeatureFlagVariantKey(client.getFeatureFlag(flag))
        })
    }, [client, flag])

    return featureFlagVariantKey
}

Maybe interesting for another PR like #717, as you could also optionally enable it like this and guarentee backwards-compatibiltiy export function useBootstrapedFeatureFlagVariantKey(flag: string, useBootstrap?: true): string | boolean | undefined