-
Notifications
You must be signed in to change notification settings - Fork 3
update posthog, add useFeatureFlagsLoaded #2720
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
Conversation
e06e72e to
dd1abc2
Compare
| const enabled = useFeatureFlagEnabled(FeatureFlags.ProductPageCourse) | ||
| if (enabled === false) { | ||
| return notFound() | ||
| const flagsLoaded = useFeatureFlagsLoaded() |
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.
If you change these lines to
const enabled = useFeatureFlagEnabled(FeatureFlags.ProductPageCourse)
const bootstrapped = useFeatureFlagEnabled(INTERNAL_BOOTSTRAPPING_FLAG)
const flagsLoaded = useFeatureFlagsLoaded()
console.log({ enabled, bootstrapped, flagsLoaded })you would see values something like
// posthog not initialized
{enabled: undefined, bootstrapped: undefined, flagsLoaded: false}
// posthog initialized, flags loaded from localstorage / bootstrapping
{enabled: true, bootstrapped: true, flagsLoaded: false}
// posthog initialized, flags loaded from server
{enabled: true, bootstrapped: undefined, flagsLoaded: true}| /** | ||
| * bootstrapFlag will be undefined: | ||
| * 1. BEFORE posthog has initialized (nothing bootstrapped yet) | ||
| * 2. AFTER posthog has loaded flags from its server. |
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.
Posthog docs have always said, apparently that posthog.isFlagEnabled (and useFlagEnabled) would return undefined if the flag is not configured on the server (e.g., a typo flag, or our bootstrapping flag here).
This actually isn't true in the version of posthog-js on main. It's kind of a breaking change, but since the docs have always described the "return undefined" behavior, they classified it as a bugfix. See PostHog/posthog-js#2276 (comment)
| * Posthog does not make detecting "flags have loaded from server" easy. | ||
| * This approach relies on the fact that bootstrapped flags are completely | ||
| * discarded after flags are loaded from server, so `useFlagEnabled` will | ||
| * return `undefined` for any flag that was only bootstrapped locally. |
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.
posthog's AI suggested this approach and it seems to work well...
| const POSTHOG_API_HOST = process.env.NEXT_PUBLIC_POSTHOG_API_HOST | ||
| const FEATURE_FLAGS = process.env.FEATURE_FLAGS | ||
|
|
||
| if (POSTHOG_API_KEY) { |
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.
For nextjs, Posthog docs recommend initializing either
instrumentation-client.ts- I tried this; it caused a bunch of hydration errors. I think that in order for the
instrumentation-client.tsapproach to work w/o hydration errors, you need also to initialize in the server and load flags server-side. Since we don't have user info on the server, we can't do that.
- I tried this; it caused a bunch of hydration errors. I think that in order for the
- provider component inside a
useEffect
The current top-level initialization caused hydration errors, too. Initializing inside the provider effect does not.
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.
Does this prevent all feature flagged content from being server rendered, even if we have environment defaults? I guess they need to be as content appearing is better than content disappearing.
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.
It does. Though that also wasn't happening with the current placement of posthog init code. On RC currently:
- code is module level
NEXT_PUBLIC_FEATURE_home_page_recommendation_botis set to true.
I do think we could get server-side flagging of the bootstrapped values to work, though.
jonkafton
left a comment
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.
Works a charm! 👍
| const POSTHOG_API_HOST = process.env.NEXT_PUBLIC_POSTHOG_API_HOST | ||
| const FEATURE_FLAGS = process.env.FEATURE_FLAGS | ||
|
|
||
| if (POSTHOG_API_KEY) { |
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.
Does this prevent all feature flagged content from being server rendered, even if we have environment defaults? I guess they need to be as content appearing is better than content disappearing.
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/9317
Description (What does it do?)
This PR fixes an issue where the feature-flagged product pages would occasionally show 404, particularly on slow networks.
How can this be tested?
product-course-pageflag enabledNODE_ENV=productioninfrontend.local.env(I find that page loads too slowly in dev mode, meaning that posthog always loads latest; the race condition is much more reproducible in production mode.)main:http://open.odl.local:8062/programs/<readable-id>NODE_ENV=production).