docs: Improve documentation/logs around PostHogProvider #1675
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| /** | ||
| * Props for the PostHogProvider component. | ||
| * This is a discriminated union type that ensures mutually exclusive props: | ||
| * | ||
| * - If `client` is provided, `apiKey` and `options` must not be provided | ||
| * - If `apiKey` is provided, `client` must not be provided, and `options` is optional | ||
| */ | ||
| type PostHogProviderProps = | ||
| | { client: PostHog; apiKey?: undefined; options?: undefined } | ||
| | { apiKey: string; options?: Partial<PostHogConfig>; client?: undefined } |
There was a problem hiding this comment.
This is somewhat of a breaking change at the type level. I know we've had other similar type-level breaking changes in the past. This is extremely easy to fix, all onboard with a minor update?
There was a problem hiding this comment.
shouldn't they be never if they aren't supposed to be provided?
will folk have to change working correct code with this change?
There was a problem hiding this comment.
shouldn't they be never if they aren't supposed to be provided?
That would work as well. The only difference is that my code allows you to call it with client={undefined} whilst never prevents it, so that's a good change, let me do that
will folk have to change working correct code with this change?
They might need to change it, but it was wrong before, so this could be considered a bugfix. We were already logging error messages to their console (but falling back to a reasonable behavior, which I kept for JS-only users).
There was a problem hiding this comment.
shouldn't they be never if they aren't supposed to be provided?
Done, force-pushed the change
There was a problem hiding this comment.
i really wish this was bundled as @posthog/react and we could version it separately
i guess this never hits people through the html snippet?
so you have to run my-package-manager update to get the change
but then we'd be making you change code without a major upgrade... which is yucky
if we were going to be good citizens we'd add a new provider and deprecate the old one... is there a not yuck way of doing that?
There was a problem hiding this comment.
i really wish this was bundled as @posthog/react and we could version it separately
Oh, absolutely. I might do that, and then we would keep this component as is, and add a big deprecation notice telling people to move to @posthog/react instead. Thoughts?
i guess this never hits people through the html snippet?
They're not using React if they're using the HTML snippet, so yeah, we can do slightly-more-dangerous stuff here if we want to. That's why I wouldn't be opposed to simply merging this and moving to the performance issue I'm delaying :)
but then we'd be making you change code without a major upgrade... which is yucky
Is it though? IMO, it's reasonable to expect someone would have to change the code if we had a bad bug, and I wouldn't argue for a major release. This isn't a bad bug, of course, but if anyone was using the Library in that way, it's kind of a bug.
if we were going to be good citizens we'd add a new provider and deprecate the old one... is there a not yuck way of doing that?
yuck all around, I'd be against this, I'd argue it's more painful for customers to understand why the heck do we have 2 providers vs. having to remove 2 props on a minor bump.
|
Size Change: 0 B Total Size: 3.27 MB ℹ️ View Unchanged
|
We're now logging more reasonable logs around our PostHogProvider to help customers identify problems with their setup. We're also typing that component much better to prevent people from getting it wrong at the type level. Inspired by #769, but not a solution
rafaeelaudibert
force-pushed
the
improve-documentation-around-posthog-provider
branch
from
ce3c250 to
9067391
Compare
rafaeelaudibert
deleted the
improve-documentation-around-posthog-provider
branch
|
Quick note here that this was definitely a breaking change. As the typescript definition changed we caught this during build so we never saw the warning and had to investigate it. The fix is trivial but at least a note in the release note would have been appreciated. That said, it's a nice change ;) |
|
hey @tiagoskaneta thanks for the feedback 💖 we want to get to a place where we can version posthog/react and posthog/web separately... the changelog is auto-generated, but we could have edited it. out of interest since the setup before allowed technically "incorrect" setup... how did this manifest for you? is it better now? |
We were using the |
We're now logging more reasonable logs around our PostHogProvider to help customers identify problems with their setup. We're also typing that component much better to prevent people from getting it wrong at the type level.
Inspired by #769, but not a solution