-
Notifications
You must be signed in to change notification settings - Fork 145
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
refactor: Simplify Posthog.init()
signature
#1712
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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
This PR updates the TypeScript signature of PostHog's init() method to correctly reflect that it always returns a PostHog instance, removing the misleading | undefined
from the return type.
Key changes:
- Modified return type of
init()
insrc/posthog-core.ts
fromPostHog | undefined
toPostHog
- Aligns TypeScript types with actual implementation behavior which already returns an instance in all cases
- Type-level breaking change that improves type safety and developer experience
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
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 assume you'll have thoughts @pauldambra
🤣 i love my js sdk, don't hurt them!
I think this is a safe direction to make the change. You're either already ignoring that this could be undefined or handling it.
At least now we'd never return undefined. Potentially as with the previous change someone's TS breaks
e.g. if they have const x: PostHog | undefined = init(blah)
then TS might complain
but importantly i think this is safe in the running code!
Size Change: 0 B Total Size: 3.3 MB ℹ️ View Unchanged
|
In a recent version we updated the body of this function to always return a `Posthog` instance but we forgot to update the type, let's fix that Originally written by https://github.com/thexeos but I couldn't get his PR to pass CI - secrets are not shared with contributors PRs Closes #1616 Co-authored by thexeos <teodor@massluminosity.com>
17d7ed8
to
f1d3acb
Compare
In a recent version, we updated the body of this function to always return a
Posthog
instance but we forgot to update the type, let's fix thatThis is a slight type-level breaking change at
posthog-core.ts
. This is more serious than the recent changes we did to code that was only present in the React SDK. Should we ship this? I assume you'll have thoughts @pauldambraOriginally written by https://github.com/thexeos but I couldn't get his PR to pass CI - secrets are not shared with contributors PRs
Closes #1616
Supersedes #1617