-
Notifications
You must be signed in to change notification settings - Fork 143
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
chore: Rework SDK initialisation #1023
Conversation
# Conflicts: # src/__tests__/posthog-core.identify.js # src/__tests__/posthog-core.js # src/extensions/sessionrecording.ts # src/posthog-core.ts # src/posthog-featureflags.ts # src/posthog-surveys.ts
# Conflicts: # src/__tests__/posthog-core.js # src/posthog-core.ts
Hey @benjackwhite! 👋 |
Size Change: -3.85 kB (0%) Total Size: 858 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.
Makes sense to me!
I hope nobody was accidentally relying on the undocumented old initialization path. Seems low risk.
# Conflicts: # src/posthog-core.ts
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
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.
LGTM
runs locally, is smaller when bundled 👨🍳 👌
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
# Conflicts: # src/__tests__/posthog-core.js
I was debugging https://posthoghelp.zendesk.com/agent/tickets/11815 and I believe this PR is the most likely root cause. The global variable Might be worth delaying the |
Changes
We get a lot of issues with people initialising PostHog incorrectly, sometimes due to think an env var is set when it isn't.
I started trying to add this and the spaghetti code of initialisation caused such a headache that I felt it was time we improved this.
Big change so we should validate this heavily but this:
TODO
Checklist