fix: don't load preact until you have to #1311
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Size Change: -29.9 kB (-2.52%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
| @@ -147,7 +162,12 @@ describe('surveys', () => { | |||
| }, | |||
| } as unknown as PostHog | |||
There was a problem hiding this comment.
to keep this PR smaller, i'm not replacing the mock posthog here
(we really should be, but it can wait)
| @@ -757,33 +779,6 @@ describe('surveys', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| describe('decide response', () => { | |||
There was a problem hiding this comment.
since we're mocking load script we don't need to test whether or not it behaves correctly...
| ;(window as any).__PosthogExtensions__ = (window as any).__PosthogExtensions__ || {} | ||
| ;(window as any).__PosthogExtensions__.canActivateRepeatedly = canActivateRepeatedly |
There was a problem hiding this comment.
maybe this should even be PosthogExtensions.Surveys but 🤷
but regardless this is the mechanism we use to push code out of the main bundle so that folk who don't use surveys (or replay or web vitals etc etc) don't have to download the code for that feature
accessing canActivateRepeatedly causes the bundler to pull in preact and you only need that with surveys enabled
| import { canActivateRepeatedly } from './extensions/surveys/surveys-utils' | ||
| import { isNullish } from './utils/type-utils' | ||
|
|
||
| const LOGGER_PREFIX = '[Surveys]' |
There was a problem hiding this comment.
QOL we put a prefix on logs so that they are easier to read
| private _decideServerResponse?: boolean | ||
| public _surveyEventReceiver: SurveyEventReceiver | null | ||
|
|
||
| constructor(instance: PostHog) { | ||
| this.instance = instance | ||
| constructor(private readonly instance: PostHog) { |
There was a problem hiding this comment.
QOL: TS lets you declare a field like this when it's a constructor parameter
| @@ -178,7 +178,7 @@ export class PostHogSurveys { | |||
| const eventBasedTargetingFlagCheck = | |||
| hasEvents || hasActions ? activatedSurveys?.includes(survey.id) : true | |||
|
|
|||
| const overrideInternalTargetingFlagCheck = canActivateRepeatedly(survey) | |||
| const overrideInternalTargetingFlagCheck = this._canActivateRepeatedly(survey) | |||
There was a problem hiding this comment.
this single line effectively reduces bundle size by 6%
| @@ -258,7 +258,15 @@ export class PostHogSurveys { | |||
| return nextQuestionIndex | |||
| } | |||
|
|
|||
| console.warn('Falling back to next question index due to unexpected branching type') // eslint-disable-line no-console | |||
There was a problem hiding this comment.
try not to disable eslint rules, here it was warning you not to use console and it was IMHO right 😊
| // this method is lazily loaded onto the window to avoid loading preact and other dependencies if surveys is not enabled | ||
| private _canActivateRepeatedly(survey: Survey) { | ||
| if (isNullish(assignableWindow.__PosthogExtensions__.canActivateRepeatedly)) { | ||
| logger.warn(LOGGER_PREFIX, 'canActivateRepeatedly is not defined, must init before calling') | ||
| } | ||
| return assignableWindow.__PosthogExtensions__.canActivateRepeatedly(survey) | ||
| } |
There was a problem hiding this comment.
convenience method for accessing the lazily loaded canActivateRepeatedly
There was a problem hiding this comment.
This change seems reasonable to me, thanks for keeping at it and making the PR! @pauldambra, Is there anything else you needed from the Feature Success team other than a second pair of eyes on the review? I kicked CI again and it passed this time so all seems good from my perspective.
we need to keep the bundle as small as possible, it's already ~3x bigger than when people first start complaining about the size
#1281 is in merge hell because we abandoned it
this is another attempt to avoid everyone downloading preact, it reduces bundle size by 6%