-
Notifications
You must be signed in to change notification settings - Fork 129
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
feat(surveys): Make surveys site app native to posthog-js #801
Conversation
Size Change: +400 B (0%) Total Size: 709 kB
ℹ️ View Unchanged
|
This is part 1 of X. This should be safe to go in as is, because until I'll test this manually, check we have it in the CDN, before making the change in app & figuring out cache busting rules. |
Surveys site app code will get updated relatively often, but now users will have to manually update their posthog js version each time to get any new updates? Hmmmm |
No, because the latest version is loaded from our servers. They can be on an older posthog-js version and still get the latest surveys, they're independent. Unless, they explicitly bundle the two together and pin it. |
Great |
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.
Overall looks good. I didn't dive into the actual "surveys" code much. I think as soon as possible we should consider moving this to some sort of lighweight frontend framework but thats for another time
src/decide.ts
Outdated
|
||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore | ||
window.generateSurveys(this.instance) |
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.
Can we scope this a little more perhaps? window.extendPostHogWithSurveys
or something?
Should be incredibly low chance of a clash but still. "generateSurveys" is somewhat too generic
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, will do 👍
let currentUrl = location.href | ||
if (location.href) { | ||
setInterval(() => { | ||
if (location.href !== currentUrl) { | ||
currentUrl = location.href | ||
callSurveys(posthog, false) | ||
} | ||
}, 1500) |
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.
Opinion incoming - this feels like something that should be in posthog-js
- like posthog.onActiveSurveysChanged()
or something. Or is it somehow unique to our GUI implementation?
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 some sense, will skip for now though and have a think after the migration
Changes
Deprecate surveys site app, and make surveys part of posthog-js, as an optional loader
https://github.com/PostHog/feature-surveys
I opted not to make an interface for
PostHog
core, because as it turns out, there's some bundling magic happening which strips the type out of the js bundle, ensuring the size remains small.Only when I do something with
PostHog
does it get bundled in.I'd recommend checking all the code except what's in
extensions/surveys.ts
because that's just a copy from the site app.A future PR might change this into a more extensible react based flow. Right now, the focus is making sure this can be loaded correctly.
(I need to write a few more loader tests, which I will soon, but wanted the PoC out for review)
...
Checklist