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
Show file tree
Hide file tree
Changes from all commits
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
53 changes: 24 additions & 29 deletions src/__tests__/surveys.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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[] = [
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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,
Expand All @@ -156,6 +176,8 @@ describe('surveys', () => {
// eslint-disable-next-line compat/compat
value: new URL('https://example.com'),
})

surveys.afterDecideResponse(decideResponse)
})

afterEach(() => {
Expand Down Expand Up @@ -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',
Expand Down
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
}

Expand Down
22 changes: 15 additions & 7 deletions src/posthog-surveys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ 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


export const surveyUrlValidationMap: Record<SurveyUrlMatchType, (conditionsUrl: string) => boolean> = {
icontains: (conditionsUrl) =>
Expand Down Expand Up @@ -50,12 +52,10 @@ function getRatingBucketForResponseValue(responseValue: number, scale: number) {
}

export class PostHogSurveys {
instance: PostHog
private _decideServerResponse?: boolean
public _surveyEventReceiver: SurveyEventReceiver | null

constructor(instance: PostHog) {
this.instance = instance
constructor(private readonly instance: 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.

QOL: TS lets you declare a field like this when it's a constructor parameter

// 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
Expand All @@ -75,7 +75,7 @@ export class PostHogSurveys {
}
this.instance.requestRouter.loadScript('/static/surveys.js', (err) => {
if (err) {
return logger.error(`Could not load surveys script`, err)
return logger.error(LOGGER_PREFIX, 'Could not load surveys script', err)
}

assignableWindow.extendPostHogWithSurveys(this.instance)
Expand Down Expand Up @@ -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)
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)
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

try not to disable eslint rules, here it was warning you not to use console and it was IMHO right 😊

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

}
Loading