Skip to content
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: don't load preact until you have to #1311

Merged
merged 3 commits into from
Jul 22, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix: don't load preact too early
pauldambra committed Jul 20, 2024
commit ef802477bd8a9db069ceb02e517e1b01813e356b
53 changes: 24 additions & 29 deletions src/__tests__/surveys.test.ts
Original file line number Diff line number Diff line change
@@ -9,14 +9,18 @@ import {
SurveyQuestionBranchingType,
SurveyQuestion,
} from '../posthog-surveys-types'
import { getDisplayOrderChoices, getDisplayOrderQuestions } from '../extensions/surveys/surveys-utils'
import {
canActivateRepeatedly,
getDisplayOrderChoices,
getDisplayOrderQuestions,
} from '../extensions/surveys/surveys-utils'
import { PostHogPersistence } from '../posthog-persistence'
import { PostHog } from '../posthog-core'
import { DecideResponse, PostHogConfig, Properties } from '../types'
import { window } from '../utils/globals'
import { RequestRouter } from '../utils/request-router'
import { assignableWindow } from '../utils/globals'
import { expectScriptToExist, expectScriptToNotExist } from './helpers/script-utils'
import { generateSurveys } from '../extensions/surveys'

describe('surveys', () => {
let config: PostHogConfig
@@ -34,6 +38,7 @@ describe('surveys', () => {
'enabled-internal-targeting-flag-key': true,
'disabled-internal-targeting-flag-key': false,
},
surveys: true,
} as unknown as DecideResponse

const firstSurveys: Survey[] = [
@@ -119,6 +124,16 @@ describe('surveys', () => {
beforeEach(() => {
surveysResponse = { surveys: firstSurveys }

const loadScriptMock = jest.fn()

loadScriptMock.mockImplementation((_path, callback) => {
assignableWindow.__PosthogExtensions__ = assignableWindow.__Posthog__ || {}
assignableWindow.extendPostHogWithSurveys = generateSurveys
assignableWindow.__PosthogExtensions__.canActivateRepeatedly = canActivateRepeatedly

callback()
})

config = {
token: 'testtoken',
api_host: 'https://app.posthog.com',
@@ -147,7 +162,12 @@ describe('surveys', () => {
},
} as unknown as PostHog
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to keep this PR smaller, i'm not replacing the mock posthog here
(we really should be, but it can wait)


instance.requestRouter.loadScript = loadScriptMock

surveys = new PostHogSurveys(instance)
instance.surveys = surveys
// all being squashed into a mock posthog so...
instance.getActiveMatchingSurveys = instance.surveys.getActiveMatchingSurveys.bind(instance.surveys)

Object.defineProperty(window, 'location', {
configurable: true,
@@ -156,6 +176,8 @@ describe('surveys', () => {
// eslint-disable-next-line compat/compat
value: new URL('https://example.com'),
})

surveys.afterDecideResponse(decideResponse)
})

afterEach(() => {
@@ -757,33 +779,6 @@ describe('surveys', () => {
})
})

describe('decide response', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we're mocking load script we don't need to test whether or not it behaves correctly...

beforeEach(() => {
// clean the JSDOM to prevent interdependencies between tests
document.body.innerHTML = ''
document.head.innerHTML = ''
})

it('should not load when decide response says no', () => {
surveys.afterDecideResponse({ surveys: false } as DecideResponse)
// Make sure the script is not loaded
expectScriptToNotExist('https://us-assets.i.posthog.com/static/surveys.js')
})

it('should load when decide response says so', () => {
surveys.afterDecideResponse({ surveys: true } as DecideResponse)
// Make sure the script is loaded
expectScriptToExist('https://us-assets.i.posthog.com/static/surveys.js')
})

it('should not load when config says no', () => {
config.disable_surveys = true
surveys.afterDecideResponse({ surveys: true } as DecideResponse)
// Make sure the script is not loaded
expectScriptToNotExist('https://us-assets.i.posthog.com/static/surveys.js')
})
})

describe('branching logic', () => {
const survey: Survey = {
name: 'My survey',
3 changes: 3 additions & 0 deletions src/entrypoints/surveys.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { generateSurveys } from '../extensions/surveys'

import { window } from '../utils/globals'
import { canActivateRepeatedly } from '../extensions/surveys/surveys-utils'

if (window) {
;(window as any).__PosthogExtensions__ = (window as any).__PosthogExtensions__ || {}
;(window as any).__PosthogExtensions__.canActivateRepeatedly = canActivateRepeatedly
Comment on lines +7 to +8
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

;(window as any).extendPostHogWithSurveys = generateSurveys
}

13 changes: 11 additions & 2 deletions src/posthog-surveys.ts
Original file line number Diff line number Diff line change
@@ -12,7 +12,7 @@ import { SurveyEventReceiver } from './utils/survey-event-receiver'
import { assignableWindow, document, window } from './utils/globals'
import { DecideResponse } from './types'
import { logger } from './utils/logger'
import { canActivateRepeatedly } from './extensions/surveys/surveys-utils'
import { isNullish } from './utils/type-utils'

const LOGGER_PREFIX = '[Surveys]'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QOL we put a prefix on logs so that they are easier to read


@@ -59,6 +59,7 @@ export class PostHogSurveys {
// we set this to undefined here because we need the persistence storage for this type
// but that's not initialized until loadIfEnabled is called.
this._surveyEventReceiver = null
this.loadIfEnabled()
}

afterDecideResponse(response: DecideResponse) {
@@ -178,7 +179,7 @@ export class PostHogSurveys {
const eventBasedTargetingFlagCheck =
hasEvents || hasActions ? activatedSurveys?.includes(survey.id) : true

const overrideInternalTargetingFlagCheck = canActivateRepeatedly(survey)
const overrideInternalTargetingFlagCheck = this._canActivateRepeatedly(survey)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this single line effectively reduces bundle size by 6%

const internalTargetingFlagCheck =
survey.internal_targeting_flag_key && !overrideInternalTargetingFlagCheck
? this.instance.featureFlags.isFeatureEnabled(survey.internal_targeting_flag_key)
@@ -261,4 +262,12 @@ export class PostHogSurveys {
logger.warn(LOGGER_PREFIX, 'Falling back to next question index due to unexpected branching type')
return nextQuestionIndex
}

// 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)
}
Comment on lines +265 to +271
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convenience method for accessing the lazily loaded canActivateRepeatedly

}