-
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
fix: improve survey bundle size v2 #1759
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
9d1bfd9
to
6429be9
Compare
Size Change: -7.08 kB (-0.21%) Total Size: 3.32 MB
ℹ️ 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
This PR optimizes bundle size by relocating survey-related logic from core files to the surveys extension.
- Moved
getNextSurveyStep
andgetRatingBucketForResponseValue
functions fromposthog-core.ts
tosrc/extensions/surveys.tsx
- Updated imports in
src/entrypoints/surveys-preview.es.ts
to reference functions directly from surveys extension - Removed old SDK/branching logic since surveys will be loaded with the extension
- Consolidated survey-specific functionality in
surveys.tsx
to prevent unnecessary code loading - Updated test imports in
src/__tests__/surveys.test.ts
to reflect new function locations
5 file(s) reviewed, 2 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.
assuming this is not breaking the frontend that depends on the next question for the survey preview, LGTM
@@ -1341,15 +1341,6 @@ export class PostHog { | |||
this.surveys.canRenderSurvey(surveyId) | |||
} | |||
|
|||
/** Get the next step of the survey: a question index or `end` */ | |||
getNextSurveyStep( |
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.
since this is on posthog core could someone have integrated with it? should we keep the method and replace it instead with a deprecation log message, then replace it after some time?
(you all the experts on surveys so i may be worrying unnecessarily)
because of the lazy loading we always have to consider someone with posthog-js main bundle pre-this-change and latest lazy bundle
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 be fine, i checked for that! ran all e2e tests on posthog/posthog and did smoke tests for surveys extensively. didn't saw any other usage besides surveys' domain
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.
one comment on how to think about avoiding breaking releases here but only to make sure you've thought about it 😊
otherwise, love this kind of analysis and CI is green 🙌
Changes
Follow up to #1665 which I'll prob close.
I realized the method we were loading lazily,
getSurveyNextStep
was only really used inside the Surveys file (extension/surveys.tsx).However, we were still loading it under the
posthog-core.ts
, which it doesn't really need.So, instead of trying to load it individually lazily, I just added it to the main surveys extension file, and it can be loaded directly with that.
It also allows us to remove the old sdk / no branching logic, since, once they update their survey, everything will come from the surveys extension.
@pauldambra tagged you as my local posthog-js expert, let me know if this reasoning makes sense
Checklist