-
Notifications
You must be signed in to change notification settings - Fork 55
feat(nextjs): create @posthog/nextjs package #515
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
base: main
Are you sure you want to change the base?
Conversation
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.
PR Summary
Introduces a new @posthog/nextjs
package that provides sourcemap generation and upload capabilities during Next.js build process, with configurable options for both server and client builds.
- Added webpack plugin in
posthog-nextjs/src/webpack-plugin.ts
for automated sourcemap injection usingposthog-cli
- Implemented flexible configuration system in
posthog-nextjs/src/config.ts
with support for async/sync Next.js configs - Missing critical documentation in
README.md
andCHANGELOG.md
that needs to be added - Package configuration in
posthog-nextjs/package.json
needs review for test dependencies and peer dependency specifications - PR template sections for bump level and affected libraries are incomplete and should be filled out
14 files reviewed, 15 comments
Edit PR Review Bot Settings | Greptile
Size Change: 0 B Total Size: 323 kB ℹ️ View Unchanged
|
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.
PR Summary
Significant changes made to the build and release infrastructure to support the new @posthog/nextjs
package.
- Added
posthog-nextjs
to release workflow matrix in.github/workflows/release.yml
for npm publishing - Rollup configuration in
rollup.config.js
expanded to build NextJS package with ESM bundle and TypeScript declarations - Package template files (PR, bug report, feature request) updated to include
posthog-nextjs
references - Root
package.json
needs updating - test scripts and version bump not properly configured - Edge runtime support marked as TODO in
webpack-plugin.ts
, needs implementation before release
13 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
"dependencies": { | ||
"@posthog/cli": "^0.1.0", | ||
"rimraf": "5.0.9" | ||
}, |
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.
Should these be peer dependencies?
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 me they are real deps. Users should not worry about how we implemented it, it could be using cli or our own js implementation.
], | ||
"devDependencies": { | ||
"@types/node": "^22.15.23", | ||
"next": "^12.1.0" |
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.
Is this need in both?
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.
Yes, peerDependencies are not installed when running 'yarn install' and you need them to dev (type checking and so on).
import { rimraf } from 'rimraf' | ||
import { spawn } from 'child_process' | ||
|
||
type NextRuntime = 'edge' | 'nodejs' | undefined |
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.
Is fluid compute a new runtime? https://vercel.com/fluid
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.
I don't think so, maybe a mix of node and edge.
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.
I'm guessing the idea is not to make this a full replacement until we have isomorphic capabilities? I would have expected that I only needed to install @posthog/nextjs
but I guess I still need posthog-node
. Happy if that's something we want to add later
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 was the initial idea but the type are too different for now. posthog.init
vs new Posthog
, capture(event)
vs capture({event})`
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.
What do we need to do to make types the same?
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.
I didn't check what needs to be changed in details but basically the idea is to make sure posthog-node
and posthog-js
have the same public API. It was clearly out of this PR scope and it should probably be part of broader effort to share more code and have an unified API design across SDKs.
@@ -0,0 +1,98 @@ | |||
import { PostHogNextConfigComplete } from './config' |
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.
Is the plan to move this out of the nextjs repo so that it can be used by other frameworks / as a bundler plugin?
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.
This webpack plugin is really next specific but we could probably make it more generic, if there is a need at some point
authToken, | ||
envId, |
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.
What are these for? They dont follow the same names we have in our other SDKs
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.
We use those variables to upload sourcemaps during the nextjs build process. I reused the ones from the CLI => https://www.npmjs.com/package/@posthog/cli
I could rename authToken to personalApiKey to SDK convention, do we use the envId anywhere ?
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.
I wonder why this is here and not on posthog-js
where we already include some stuff for NextJS - such as a PostHogProvider
I'm slightly skeptical of releasing this as is and getting people to use it but it doesnt contain all of the nice stuff we already have on @posthog-js/react
meant to be used with Next. Docs will also be crazy. Is the reason behind this to simplify error tracking setup for NextJS? I think we need a solution that's easily extensible rather than a one-off
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.
What do we need to do to make types the same?
It seemed more natural to put it here as this repository is a workspace and allows the creation and publication of new packages easily. Also the initial goal was to make this library isomorphic and I needed to import The If we don't want to release this package yet because we want to put more thoughts into it, I don't mind distributing those features as an entrypoint inside the import {withPosthHogConfig} from "posthog-node/nextjs" |
Changes
Release info Sub-libraries affected
Bump level
Libraries affected
Changelog notes