Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frontends/main/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"next-nprogress-bar": "^2.4.2",
"ol-components": "0.0.0",
"ol-utilities": "0.0.0",
"posthog-js": "^1.157.2",
"posthog-js": "^1.297.2",
"react": "^19.0.0",
"react-dom": "^19.0.0",
"react-slick": "^0.30.2",
Expand Down
20 changes: 15 additions & 5 deletions frontends/main/src/app-pages/ProductPages/CoursePage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,13 @@ import { assertHeadings } from "ol-test-utilities"
import { notFound } from "next/navigation"

import { useFeatureFlagEnabled } from "posthog-js/react"
import { useFeatureFlagsLoaded } from "@/common/useFeatureFlagsLoaded"
import invariant from "tiny-invariant"

jest.mock("posthog-js/react")
const mockedUseFeatureFlagEnabled = jest.mocked(useFeatureFlagEnabled)
jest.mock("@/common/useFeatureFlagsLoaded")
const mockedUseFeatureFlagsLoaded = jest.mocked(useFeatureFlagsLoaded)

const makeCourse = factories.courses.course
const makePage = factories.pages.coursePageItem
Expand Down Expand Up @@ -46,12 +49,19 @@ const setupApis = ({
describe("CoursePage", () => {
beforeEach(() => {
mockedUseFeatureFlagEnabled.mockReturnValue(true)
mockedUseFeatureFlagsLoaded.mockReturnValue(true)
})

test.each([true, false])(
test.each([
{ flagsLoaded: true, isEnabled: true, shouldNotFound: false },
{ flagsLoaded: true, isEnabled: false, shouldNotFound: true },
{ flagsLoaded: false, isEnabled: true, shouldNotFound: false },
{ flagsLoaded: false, isEnabled: false, shouldNotFound: false },
])(
"Calls noFound if and only the feature flag is disabled",
async (isEnabled) => {
async ({ flagsLoaded, isEnabled, shouldNotFound }) => {
mockedUseFeatureFlagEnabled.mockReturnValue(isEnabled)
mockedUseFeatureFlagsLoaded.mockReturnValue(flagsLoaded)

const course = makeCourse()
const page = makePage({ course_details: course })
Expand All @@ -60,10 +70,10 @@ describe("CoursePage", () => {
url: `/courses/${course.readable_id}/`,
})

if (isEnabled) {
expect(notFound).not.toHaveBeenCalled()
} else {
if (shouldNotFound) {
expect(notFound).toHaveBeenCalled()
} else {
expect(notFound).not.toHaveBeenCalled()
}
},
)
Expand Down
8 changes: 5 additions & 3 deletions frontends/main/src/app-pages/ProductPages/CoursePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import ProductPageTemplate, {
} from "./ProductPageTemplate"
import { CoursePageItem } from "@mitodl/mitxonline-api-axios/v2"
import { DEFAULT_RESOURCE_IMG } from "ol-utilities"
import { useFeatureFlagsLoaded } from "@/common/useFeatureFlagsLoaded"

type CoursePageProps = {
readableId: string
Expand Down Expand Up @@ -76,10 +77,11 @@ const CoursePage: React.FC<CoursePageProps> = ({ readableId }) => {
const page = pages.data?.items[0]
const course = courses.data?.results?.[0]
const enabled = useFeatureFlagEnabled(FeatureFlags.ProductPageCourse)
if (enabled === false) {
return notFound()
const flagsLoaded = useFeatureFlagsLoaded()

if (!enabled) {
return flagsLoaded ? notFound() : null
}
if (!enabled) return

const doneLoading = pages.isSuccess && courses.isSuccess

Expand Down
19 changes: 14 additions & 5 deletions frontends/main/src/app-pages/ProductPages/ProgramPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ import { notFound } from "next/navigation"
import { useFeatureFlagEnabled } from "posthog-js/react"
import invariant from "tiny-invariant"
import { ResourceTypeEnum } from "api"
import { useFeatureFlagsLoaded } from "@/common/useFeatureFlagsLoaded"

jest.mock("posthog-js/react")
const mockedUseFeatureFlagEnabled = jest.mocked(useFeatureFlagEnabled)
jest.mock("@/common/useFeatureFlagsLoaded")
const mockedUseFeatureFlagsLoaded = jest.mocked(useFeatureFlagsLoaded)

const makeProgram = factories.programs.program
const makePage = factories.pages.programPageItem
Expand Down Expand Up @@ -67,10 +70,16 @@ describe("ProgramPage", () => {
mockedUseFeatureFlagEnabled.mockReturnValue(true)
})

test.each([true, false])(
test.each([
{ flagsLoaded: true, isEnabled: true, shouldNotFound: false },
{ flagsLoaded: true, isEnabled: false, shouldNotFound: true },
{ flagsLoaded: false, isEnabled: true, shouldNotFound: false },
{ flagsLoaded: false, isEnabled: false, shouldNotFound: false },
])(
"Calls noFound if and only the feature flag is disabled",
async (isEnabled) => {
async ({ flagsLoaded, isEnabled, shouldNotFound }) => {
mockedUseFeatureFlagEnabled.mockReturnValue(isEnabled)
mockedUseFeatureFlagsLoaded.mockReturnValue(flagsLoaded)

const program = makeProgram()
const page = makePage({ program_details: program })
Expand All @@ -79,10 +88,10 @@ describe("ProgramPage", () => {
url: `/programs/${program.readable_id}/`,
})

if (isEnabled) {
expect(notFound).not.toHaveBeenCalled()
} else {
if (shouldNotFound) {
expect(notFound).toHaveBeenCalled()
} else {
expect(notFound).not.toHaveBeenCalled()
}
},
)
Expand Down
8 changes: 5 additions & 3 deletions frontends/main/src/app-pages/ProductPages/ProgramPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { ProgramSummary } from "./ProductSummary"
import { DEFAULT_RESOURCE_IMG } from "ol-utilities"
import { learningResourceQueries } from "api/hooks/learningResources"
import { ResourceTypeEnum } from "api"
import { useFeatureFlagsLoaded } from "@/common/useFeatureFlagsLoaded"

type ProgramPageProps = {
readableId: string
Expand Down Expand Up @@ -89,10 +90,11 @@ const ProgramPage: React.FC<ProgramPageProps> = ({ readableId }) => {
const program = programs.data?.results?.[0]
const programResource = programResources.data?.results?.[0]
const enabled = useFeatureFlagEnabled(FeatureFlags.ProductPageCourse)
if (enabled === false) {
return notFound()
const flagsLoaded = useFeatureFlagsLoaded()
Copy link
Contributor Author

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}


if (!enabled) {
return flagsLoaded ? notFound() : null
}
if (!enabled) return

const isLoading =
pages.isLoading || programs.isLoading || programResources.isLoading
Expand Down
9 changes: 9 additions & 0 deletions frontends/main/src/common/feature_flags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,12 @@ export enum FeatureFlags {
VideoShorts = "video-shorts",
ProductPageCourse = "product-page-course",
}

/**
* A special flag that indicates feature flags are in their bootstrapped state,
* not yet loaded from PostHog server.
*
* DO NOT add this flag to PostHog!
*/
export const INTERNAL_BOOTSTRAPPING_FLAG =
"__flags_are_bootstrapped_do_not_add_this_to_posthog__"
53 changes: 53 additions & 0 deletions frontends/main/src/common/useFeatureFlagsLoaded.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { INTERNAL_BOOTSTRAPPING_FLAG } from "@/common/feature_flags"
import { useFeatureFlagEnabled, usePostHog } from "posthog-js/react"
import { useEffect, useState } from "react"

/**
* Returns `true` if feature flags have been loaded via posthog API, else `false`.
*
* NOTES:
* 1. Avoid using this! Really!:
* - This can delay feature flag availability for users who have been to the
* site before. Their flags will be bootstrapped from localStorage, and should
* have the correct values immediately, before contacting the PostHog server.
* - We generally shouldn't care if a flag is "false" or
* "not loaded yet".
*
* USE CASE: One case where this distinction matters is when an entire page
* is behind a feature flag, and we don't want to 404 until the flags are
* loaded.
* 2. Unlike posthog's `onFeatureFlags` callback, this hook enables you to
* distinguish between bootstrapped flags and flags loaded from PostHog server.
*
* IMPLEMENTATION:
* 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.
Copy link
Contributor Author

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 useFeatureFlagsLoaded = () => {
const posthog = usePostHog()
const [hasBootstrapped, setHasBootstrapped] = useState(false)
useEffect(() => {
/**
* NOTE: this does not indicate flags have loaded from server, so they might
* be incomplete. It does at least indicate that localstorage/bootstrapped
* flags are ready
*
* If this event listener is added after posthog has fully loaded flags,
* the callback is still fired.
*/
return posthog.onFeatureFlags(() => {
setHasBootstrapped(true)
})
}, [posthog])
const bootstrapFlag = useFeatureFlagEnabled(INTERNAL_BOOTSTRAPPING_FLAG)
/**
* bootstrapFlag will be undefined:
* 1. BEFORE posthog has initialized (nothing bootstrapped yet)
* 2. AFTER posthog has loaded flags from its server.
Copy link
Contributor Author

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)

*/
return hasBootstrapped && bootstrapFlag === undefined
}

export { useFeatureFlagsLoaded }
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,12 @@ import React, { useEffect } from "react"
import posthog from "posthog-js"
import { PostHogProvider, usePostHog } from "posthog-js/react"
import { useUserMe } from "api/hooks/user"
import { INTERNAL_BOOTSTRAPPING_FLAG } from "@/common/feature_flags"

const POSTHOG_API_KEY = process.env.NEXT_PUBLIC_POSTHOG_API_KEY
const POSTHOG_API_HOST = process.env.NEXT_PUBLIC_POSTHOG_API_HOST
const FEATURE_FLAGS = process.env.FEATURE_FLAGS

if (POSTHOG_API_KEY) {
Copy link
Contributor Author

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.ts approach 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.
  • provider component inside a useEffect

The current top-level initialization caused hydration errors, too. Initializing inside the provider effect does not.

Copy link
Contributor

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.

Copy link
Contributor Author

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_bot is set to true.

I do think we could get server-side flagging of the bootstrapped values to work, though.

posthog.init(POSTHOG_API_KEY, {
api_host: POSTHOG_API_HOST,
bootstrap: {
featureFlags: FEATURE_FLAGS ? JSON.parse(FEATURE_FLAGS) : null,
},
})
}

const PosthogIdentifier = () => {
const { data: user } = useUserMe()
const posthog = usePostHog()
Expand Down Expand Up @@ -45,6 +37,22 @@ const PosthogIdentifier = () => {
const ConfiguredPostHogProvider: React.FC<{ children: React.ReactNode }> = ({
children,
}) => {
useEffect(() => {
if (POSTHOG_API_KEY) {
posthog.init(POSTHOG_API_KEY, {
api_host: POSTHOG_API_HOST,
bootstrap: {
featureFlags: FEATURE_FLAGS
? {
...JSON.parse(FEATURE_FLAGS),
[INTERNAL_BOOTSTRAPPING_FLAG]: true,
}
: null,
},
})
}
}, [])

if (!POSTHOG_API_KEY) {
return children
}
Expand Down
40 changes: 29 additions & 11 deletions yarn.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading