Skip to content

Conversation

@jkachel
Copy link
Contributor

@jkachel jkachel commented Oct 2, 2024

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5411

Description (What does it do?)

The PostHog integration was not implemented in the NextJS build of Learn. This adds it back:

  • Specifying the PostHog project API key and root URL (this is actually technically new)
  • The root-level PostHogProvider, initialized only if there's an API key set
  • Bootstrapping the provider with feature flag settings via .env if we want to do that, and customizing the prefix for those flags
  • Capturing both automatic PostHog events and lrd_view events

Any future work that gets pulled in and uses the PostHog hook or components should now "just work" too. (Unless it's like the lrd_view code, which checks to see if PostHog is there or not.)

Screenshots (if appropriate):

Components when not enabled:
ph-nextjs-disabled

Components and settings when enabled (this is with a test flag added too):
ph-nextjs-enabled

How can this be tested?

Automated tests should pass. This includes a couple for LearningResourceDrawer that were marked as skip because there wasn't PostHog support yet.

Set the required environment variables:

  • NEXT_PUBLIC_POSTHOG_PROJECT_API_KEY - the PostHog project API key

Set the optional environment variables:

  • NEXT_PUBLIC_POSTHOG_API_HOST - the PostHog API host; defaults to https://us.i.posthog.com. Don't change this unless you have your own PostHog instance running.
  • NEXT_PUBLIC_POSTHOG_FEATURE_PREFIX - the prefix to use for feature flags, defaults to FEATURE_
  • NEXT_PUBLIC_FEATURE_some_flag_name - customize the lower case bit, and change FEATURE_ if you set the prefix above; set this to something you'll recognize

With at least the required variable set, you should see the PostHogProvider show up in the React components list as in the screenshots. Additionally, you should start to see events flowing into PostHog as you start doing things, including lrd_view events (which fire when you open a learning resource drawer).

If you've bootstrapped some feature flags, you should see those get populated in the PostHogProvider options. We don't have any configured flags right now - you can manually test this by adding something that's flagged out to an existing page, but it's probably sufficient to see the options passed into the provider correctly.

If you want to test the API host setting, you can maybe point it to something like catch-webhooks to make sure it's trying to hit the right place.

- Now pull the API key and host from `process.env`
- Now pull API host - we weren't doing this before; you probably don't want to specify this, but if you want to self-host Pos
tHog (it is an option!) you'd need to be able to specify this.
- Roll feature flag processing into the new Webpack Config
- Minor update to the LearningResourceDrawer to make the custom `lrd_view` hook there work

Going forward, things that use PostHog through the usePostHog hook (or one of their components) should work as expected. (The lrd_view one did some checking to make sure PostHog was turned on, hence it needed a bit more work.)
@jkachel jkachel added Needs Review An open Pull Request that is ready for review product:mit-open Issues related to the MIT Open product labels Oct 2, 2024
@jkachel jkachel changed the base branch from main to nextjs October 2, 2024 21:07
@jkachel
Copy link
Contributor Author

jkachel commented Oct 2, 2024

pre-commit.ci run

@ChristopherChudzicki ChristopherChudzicki self-assigned this Oct 3, 2024
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great. Two small requests.

new webpack.IgnorePlugin({
resourceRegExp: /mockAxios\.ts/,
}),
new webpack.EnvironmentPlugin(processFeatureFlags()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid adding to the webpack config unless we really need to, and NextJS has an env key for this... Could processFeatureFlags return the bootstrapFeatureFlags, and then do:

const nextConfig = {
  webpack: { ... } // leave as is... I'm pretty sure we can get rid of it entirely, but no need to futz with it here.
  env: {
    FEATURE_FLAGS: JSON.stringify(bootstrapFeatureFlags)
  }
  ...
}

Comment on lines 35 to 41
return apiKey.length > 0 ? (
<PostHogProvider apiKey={apiKey} options={postHogOptions}>
{interiorElements}
</PostHogProvider>
) : (
interiorElements
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you think of doing something like

const ConfiguredPHProvider: React.FC<{ children: React.ReactNode }> = ({
  children,
}) => {
  const apiKey = process.env.NEXT_PUBLIC_POSTHOG_PROJECT_API_KEY || ""

  const apiHost =
    process.env.NEXT_PUBLIC_POSTHOG_API_HOST || "https://us.i.posthog.com"
  const featureFlags = JSON.parse(process.env.FEATURE_FLAGS || "")

  const postHogOptions = {
    api_host: apiHost,
    bootstrap: {
      featureFlags: featureFlags,
    },
  }
  if (!apiKey) return children
  return (
    <PostHogProvider apiKey={apiKey} options={postHogOptions}>
      {children}
    </PostHogProvider>
  )
}

?

Then ConfiguredPHProvider can handle the conditional itself + configuration.

Partly suggesting this to encapsulate it, but partly because we could run into the same situation in future with other providers, and as-structured I think it would get awkward with more than 1.

const apiKey = process.env.NEXT_PUBLIC_POSTHOG_PROJECT_API_KEY

useEffect(() => {
if (!apiKey || apiKey.length < 1) return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"" is falsy, so you can just check !apiKey

.oneOf(["true", "false"]),
NEXT_PUBLIC_CSRF_COOKIE_NAME: yup.string().required(),
NEXT_PUBLIC_POSTHOG_PROJECT_ID: yup.string(),
NEXT_PUBLIC_POSTHOG_PERSONAL_API_KEY: yup.string(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is NEXT_PUBLIC_POSTHOG_PERSONAL_API_KEY used for anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, and actually that shouldn't be in the build since it's the actually secret one. I pulled it (and the project ID; we don't use that either, PostHog figures it out just from the project key).

@jkachel
Copy link
Contributor Author

jkachel commented Oct 3, 2024

90ad486 addresses the comments. This does pull the PostHog provider stuff into its own component, which makes the providers.tsx a good bit cleaner.

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jkachel jkachel merged commit b7e25c9 into nextjs Oct 3, 2024
11 checks passed
@jkachel jkachel deleted the jkachel/5411-migrate-posthog-settings-for-nextjs branch October 3, 2024 17:20
@odlbot odlbot mentioned this pull request Oct 22, 2024
74 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review product:mit-open Issues related to the MIT Open product

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants