-
Notifications
You must be signed in to change notification settings - Fork 139
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
|
||
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', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||
|
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
;(window as any).extendPostHogWithSurveys = generateSurveys | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) => | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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) | ||
|
@@ -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. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. convenience method for accessing the lazily loaded canActivateRepeatedly |
||
} |
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.
to keep this PR smaller, i'm not replacing the mock posthog here
(we really should be, but it can wait)