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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 22, 2025
Merged

fix: improve survey bundle size v2 #1759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6429be9
fix: improve survey bundle size
lucasheriques Feb 22, 2025
10baec3
merge main
lucasheriques Feb 22, 2025
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
74 changes: 36 additions & 38 deletions src/__tests__/surveys.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/// <reference lib="dom" />

import { SURVEYS_REQUEST_TIMEOUT_MS } from '../constants'
import { generateSurveys } from '../extensions/surveys'
import { generateSurveys, getNextSurveyStep } from '../extensions/surveys'
import {
canActivateRepeatedly,
getDisplayOrderChoices,
Expand Down Expand Up @@ -927,8 +927,8 @@ describe('surveys', () => {
{ type: SurveyQuestionType.Open, question: 'Question A' },
{ type: SurveyQuestionType.Open, question: 'Question B' },
] as SurveyQuestion[]
expect(surveys.getNextSurveyStep(survey, 0, 'Some response')).toEqual(1)
expect(surveys.getNextSurveyStep(survey, 1, 'Some response')).toEqual(SurveyQuestionBranchingType.End)
expect(getNextSurveyStep(survey, 0, 'Some response')).toEqual(1)
expect(getNextSurveyStep(survey, 1, 'Some response')).toEqual(SurveyQuestionBranchingType.End)
})

it('should branch out to `end`', () => {
Expand All @@ -940,7 +940,7 @@ describe('surveys', () => {
},
{ type: SurveyQuestionType.Open, question: 'Question B' },
] as SurveyQuestion[]
expect(surveys.getNextSurveyStep(survey, 0, 'Some response')).toEqual(SurveyQuestionBranchingType.End)
expect(getNextSurveyStep(survey, 0, 'Some response')).toEqual(SurveyQuestionBranchingType.End)
})

it('should branch out to a specific question', () => {
Expand All @@ -953,7 +953,7 @@ describe('surveys', () => {
{ type: SurveyQuestionType.Open, question: 'Question B' },
{ type: SurveyQuestionType.Open, question: 'Question C' },
] as SurveyQuestion[]
expect(surveys.getNextSurveyStep(survey, 0, 'Some response')).toEqual(2)
expect(getNextSurveyStep(survey, 0, 'Some response')).toEqual(2)
})

// Single-choice
Expand All @@ -973,9 +973,9 @@ describe('surveys', () => {
{ type: SurveyQuestionType.Open, question: 'Why no?' },
{ type: SurveyQuestionType.Open, question: 'Why maybe?' },
] as unknown[] as SurveyQuestion[]
expect(surveys.getNextSurveyStep(survey, 0, 'Yes')).toEqual(1)
expect(surveys.getNextSurveyStep(survey, 0, 'No')).toEqual(2)
expect(surveys.getNextSurveyStep(survey, 0, 'Maybe')).toEqual(3)
expect(getNextSurveyStep(survey, 0, 'Yes')).toEqual(1)
expect(getNextSurveyStep(survey, 0, 'No')).toEqual(2)
expect(getNextSurveyStep(survey, 0, 'Maybe')).toEqual(3)
})

// Response-based branching, scale 1-3
Expand All @@ -995,9 +995,9 @@ describe('surveys', () => {
{ type: SurveyQuestionType.Open, question: 'Glad to hear that. Tell us more!' },
] as unknown[] as SurveyQuestion[]

expect(surveys.getNextSurveyStep(survey, 0, 1)).toEqual(1)
expect(surveys.getNextSurveyStep(survey, 0, 2)).toEqual(2)
expect(surveys.getNextSurveyStep(survey, 0, 3)).toEqual(3)
expect(getNextSurveyStep(survey, 0, 1)).toEqual(1)
expect(getNextSurveyStep(survey, 0, 2)).toEqual(2)
expect(getNextSurveyStep(survey, 0, 3)).toEqual(3)
})

// Response-based branching, scale 1-5
Expand All @@ -1017,9 +1017,9 @@ describe('surveys', () => {
{ type: SurveyQuestionType.Open, question: 'Glad to hear that. Tell us more!' },
] as unknown as SurveyQuestion[]

expect(surveys.getNextSurveyStep(survey, 0, 1)).toEqual(1)
expect(surveys.getNextSurveyStep(survey, 0, 3)).toEqual(2)
expect(surveys.getNextSurveyStep(survey, 0, 5)).toEqual(3)
expect(getNextSurveyStep(survey, 0, 1)).toEqual(1)
expect(getNextSurveyStep(survey, 0, 3)).toEqual(2)
expect(getNextSurveyStep(survey, 0, 5)).toEqual(3)
})

// Response-based branching, scale 1-7
Expand All @@ -1039,13 +1039,13 @@ describe('surveys', () => {
{ type: SurveyQuestionType.Open, question: 'Great! What did you enjoy the most?' },
] as unknown as SurveyQuestion[]

expect(surveys.getNextSurveyStep(survey, 0, 1)).toEqual(1)
expect(surveys.getNextSurveyStep(survey, 0, 2)).toEqual(1)
expect(surveys.getNextSurveyStep(survey, 0, 3)).toEqual(1)
expect(surveys.getNextSurveyStep(survey, 0, 4)).toEqual(2)
expect(surveys.getNextSurveyStep(survey, 0, 5)).toEqual(3)
expect(surveys.getNextSurveyStep(survey, 0, 6)).toEqual(3)
expect(surveys.getNextSurveyStep(survey, 0, 7)).toEqual(3)
expect(getNextSurveyStep(survey, 0, 1)).toEqual(1)
expect(getNextSurveyStep(survey, 0, 2)).toEqual(1)
expect(getNextSurveyStep(survey, 0, 3)).toEqual(1)
expect(getNextSurveyStep(survey, 0, 4)).toEqual(2)
expect(getNextSurveyStep(survey, 0, 5)).toEqual(3)
expect(getNextSurveyStep(survey, 0, 6)).toEqual(3)
expect(getNextSurveyStep(survey, 0, 7)).toEqual(3)
})

// Response-based branching, scale 0-10 (NPS)
Expand All @@ -1065,9 +1065,9 @@ describe('surveys', () => {
{ type: SurveyQuestionType.Open, question: 'Glad to hear that. Tell us more!' },
] as unknown as SurveyQuestion[]

expect(surveys.getNextSurveyStep(survey, 0, 1)).toEqual(1)
expect(surveys.getNextSurveyStep(survey, 0, 8)).toEqual(2)
expect(surveys.getNextSurveyStep(survey, 0, 10)).toEqual(3)
expect(getNextSurveyStep(survey, 0, 1)).toEqual(1)
expect(getNextSurveyStep(survey, 0, 8)).toEqual(2)
expect(getNextSurveyStep(survey, 0, 10)).toEqual(3)
})

it('should display questions in the order AGCEHDFB', () => {
Expand Down Expand Up @@ -1121,7 +1121,7 @@ describe('surveys', () => {
for (let i = 0; i < survey.questions.length; i++) {
const currentQuestion = survey.questions[currentStep]
actualOrder.push(currentQuestion.question)
currentStep = surveys.getNextSurveyStep(survey, currentStep, 'Some response')
currentStep = getNextSurveyStep(survey, currentStep, 'Some response')
}

expect(desiredOrder).toEqual(actualOrder)
Expand Down Expand Up @@ -1180,7 +1180,7 @@ describe('surveys', () => {
for (const answer of answers) {
const currentQuestion = survey.questions[currentStep]
actualOrder.push(currentQuestion.question)
currentStep = surveys.getNextSurveyStep(survey, currentStep, answer)
currentStep = getNextSurveyStep(survey, currentStep, answer)
}
expect(desiredOrder).toEqual(actualOrder)
expect(currentStep).toEqual(SurveyQuestionBranchingType.End)
Expand All @@ -1193,7 +1193,7 @@ describe('surveys', () => {
for (const answer of answers) {
const currentQuestion = survey.questions[currentStep]
actualOrder.push(currentQuestion.question)
currentStep = surveys.getNextSurveyStep(survey, currentStep, answer)
currentStep = getNextSurveyStep(survey, currentStep, answer)
}
expect(desiredOrder).toEqual(actualOrder)
expect(currentStep).toEqual(SurveyQuestionBranchingType.End)
Expand All @@ -1206,7 +1206,7 @@ describe('surveys', () => {
for (const answer of answers) {
const currentQuestion = survey.questions[currentStep]
actualOrder.push(currentQuestion.question)
currentStep = surveys.getNextSurveyStep(survey, currentStep, answer)
currentStep = getNextSurveyStep(survey, currentStep, answer)
}
expect(desiredOrder).toEqual(actualOrder)
expect(currentStep).toEqual(SurveyQuestionBranchingType.End)
Expand All @@ -1223,7 +1223,7 @@ describe('surveys', () => {
for (const answer of answers) {
const currentQuestion = survey.questions[currentStep]
actualOrder.push(currentQuestion.question)
currentStep = surveys.getNextSurveyStep(survey, currentStep, answer)
currentStep = getNextSurveyStep(survey, currentStep, answer)
}
expect(desiredOrder).toEqual(actualOrder)
expect(currentStep).toEqual(SurveyQuestionBranchingType.End)
Expand All @@ -1244,7 +1244,7 @@ describe('surveys', () => {
{ type: SurveyQuestionType.Open, question: 'Seems you are not completely happy. Tell us more!' },
{ type: SurveyQuestionType.Open, question: 'Glad to hear that. Tell us more!' },
] as unknown as SurveyQuestion[]
expect(() => surveys.getNextSurveyStep(survey, 0, 1)).toThrow('The scale must be one of: 3, 5, 7, 10')
expect(() => getNextSurveyStep(survey, 0, 1)).toThrow('The scale must be one of: 3, 5, 7, 10')
})

it('should throw an error for a response value out of the valid range', () => {
Expand All @@ -1262,13 +1262,13 @@ describe('surveys', () => {
{ type: SurveyQuestionType.Open, question: 'Seems you are not completely happy. Tell us more!' },
{ type: SurveyQuestionType.Open, question: 'Glad to hear that. Tell us more!' },
] as unknown as SurveyQuestion[]
expect(() => surveys.getNextSurveyStep(survey, 0, 20)).toThrow('The response must be in range 1-3')
expect(() => getNextSurveyStep(survey, 0, 20)).toThrow('The response must be in range 1-3')
;(survey.questions[0] as RatingSurveyQuestion).scale = 5
expect(() => surveys.getNextSurveyStep(survey, 0, 20)).toThrow('The response must be in range 1-5')
expect(() => getNextSurveyStep(survey, 0, 20)).toThrow('The response must be in range 1-5')
;(survey.questions[0] as RatingSurveyQuestion).scale = 7
expect(() => surveys.getNextSurveyStep(survey, 0, 20)).toThrow('The response must be in range 1-7')
expect(() => getNextSurveyStep(survey, 0, 20)).toThrow('The response must be in range 1-7')
;(survey.questions[0] as RatingSurveyQuestion).scale = 10
expect(() => surveys.getNextSurveyStep(survey, 0, 20)).toThrow('The response must be in range 0-10')
expect(() => getNextSurveyStep(survey, 0, 20)).toThrow('The response must be in range 0-10')
})

it('should throw an error for if a response value in a rating question is not an integer', () => {
Expand All @@ -1286,10 +1286,8 @@ describe('surveys', () => {
{ type: SurveyQuestionType.Open, question: 'Seems you are not completely happy. Tell us more!' },
{ type: SurveyQuestionType.Open, question: 'Glad to hear that. Tell us more!' },
] as unknown as SurveyQuestion[]
expect(() => surveys.getNextSurveyStep(survey, 0, '2')).toThrow('The response type must be an integer')
expect(() => surveys.getNextSurveyStep(survey, 0, 'some_string')).toThrow(
'The response type must be an integer'
)
expect(() => getNextSurveyStep(survey, 0, '2')).toThrow('The response type must be an integer')
expect(() => getNextSurveyStep(survey, 0, 'some_string')).toThrow('The response type must be an integer')
})
})

Expand Down
3 changes: 1 addition & 2 deletions src/entrypoints/surveys-preview.es.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
export { renderFeedbackWidgetPreview, renderSurveysPreview } from '../extensions/surveys'
export { getNextSurveyStep } from '../posthog-surveys'
export { getNextSurveyStep, renderFeedbackWidgetPreview, renderSurveysPreview } from '../extensions/surveys'
116 changes: 104 additions & 12 deletions src/extensions/surveys.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,109 @@ const logger = createLogger('[Surveys]')
const window = _window as Window & typeof globalThis
const document = _document as Document

function getRatingBucketForResponseValue(responseValue: number, scale: number) {
if (scale === 3) {
if (responseValue < 1 || responseValue > 3) {
throw new Error('The response must be in range 1-3')
}

return responseValue === 1 ? 'negative' : responseValue === 2 ? 'neutral' : 'positive'
} else if (scale === 5) {
if (responseValue < 1 || responseValue > 5) {
throw new Error('The response must be in range 1-5')
}

return responseValue <= 2 ? 'negative' : responseValue === 3 ? 'neutral' : 'positive'
} else if (scale === 7) {
if (responseValue < 1 || responseValue > 7) {
throw new Error('The response must be in range 1-7')
}

return responseValue <= 3 ? 'negative' : responseValue === 4 ? 'neutral' : 'positive'
} else if (scale === 10) {
if (responseValue < 0 || responseValue > 10) {
throw new Error('The response must be in range 0-10')
}

return responseValue <= 6 ? 'detractors' : responseValue <= 8 ? 'passives' : 'promoters'
}

throw new Error('The scale must be one of: 3, 5, 7, 10')
}

export function getNextSurveyStep(
survey: Survey,
currentQuestionIndex: number,
response: string | string[] | number | null
) {
const question = survey.questions[currentQuestionIndex]
const nextQuestionIndex = currentQuestionIndex + 1

if (!question.branching?.type) {
if (currentQuestionIndex === survey.questions.length - 1) {
return SurveyQuestionBranchingType.End
}

return nextQuestionIndex
}

if (question.branching.type === SurveyQuestionBranchingType.End) {
return SurveyQuestionBranchingType.End
} else if (question.branching.type === SurveyQuestionBranchingType.SpecificQuestion) {
if (Number.isInteger(question.branching.index)) {
return question.branching.index
}
} else if (question.branching.type === SurveyQuestionBranchingType.ResponseBased) {
// Single choice
if (question.type === SurveyQuestionType.SingleChoice) {
// :KLUDGE: for now, look up the choiceIndex based on the response
// TODO: once QuestionTypes.MultipleChoiceQuestion is refactored, pass the selected choiceIndex into this method
const selectedChoiceIndex = question.choices.indexOf(`${response}`)

if (question.branching?.responseValues?.hasOwnProperty(selectedChoiceIndex)) {
const nextStep = question.branching.responseValues[selectedChoiceIndex]

// Specific question
if (Number.isInteger(nextStep)) {
return nextStep
}

if (nextStep === SurveyQuestionBranchingType.End) {
return SurveyQuestionBranchingType.End
}

return nextQuestionIndex
}
} else if (question.type === SurveyQuestionType.Rating) {
if (typeof response !== 'number' || !Number.isInteger(response)) {
throw new Error('The response type must be an integer')
}

const ratingBucket = getRatingBucketForResponseValue(response, question.scale)

if (question.branching?.responseValues?.hasOwnProperty(ratingBucket)) {
const nextStep = question.branching.responseValues[ratingBucket]

// Specific question
if (Number.isInteger(nextStep)) {
return nextStep
}

if (nextStep === SurveyQuestionBranchingType.End) {
return SurveyQuestionBranchingType.End
}

return nextQuestionIndex
}
}

return nextQuestionIndex
}

logger.warn('Falling back to next question index due to unexpected branching type')
return nextQuestionIndex
}

export class SurveyManager {
private posthog: PostHog
private surveyInFocus: string | null
Expand Down Expand Up @@ -663,18 +766,7 @@ export function Questions({

setQuestionsResponses({ ...questionsResponses, [responseKey]: res })

// Old SDK, no branching
if (!posthog.getNextSurveyStep) {
const isLastDisplayedQuestion = displayQuestionIndex === survey.questions.length - 1
if (isLastDisplayedQuestion) {
sendSurveyEvent({ ...questionsResponses, [responseKey]: res }, survey, posthog)
} else {
setCurrentQuestionIndex(displayQuestionIndex + 1)
}
return
}

const nextStep = posthog.getNextSurveyStep(survey, displayQuestionIndex, res)
const nextStep = getNextSurveyStep(survey, displayQuestionIndex, res)
if (nextStep === SurveyQuestionBranchingType.End) {
sendSurveyEvent({ ...questionsResponses, [responseKey]: res }, survey, posthog)
} else {
Expand Down
11 changes: 1 addition & 10 deletions src/posthog-core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { PostHogExceptions } from './posthog-exceptions'
import { PostHogFeatureFlags } from './posthog-featureflags'
import { PostHogPersistence } from './posthog-persistence'
import { PostHogSurveys } from './posthog-surveys'
import { Survey, SurveyCallback, SurveyQuestionBranchingType } from './posthog-surveys-types'
import { SurveyCallback } from './posthog-surveys-types'
import { RateLimiter } from './rate-limiter'
import { RemoteConfigLoader } from './remote-config'
import { extendURLParams, request, SUPPORTS_REQUEST } from './request'
Expand Down Expand Up @@ -1343,15 +1343,6 @@ export class PostHog {
this.surveys.canRenderSurvey(surveyId)
}

/** Get the next step of the survey: a question index or `end` */
getNextSurveyStep(
Copy link
Member

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

Copy link
Contributor Author

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

survey: Survey,
currentQuestionIndex: number,
response: string | string[] | number | null
): number | SurveyQuestionBranchingType.End {
return this.surveys.getNextSurveyStep(survey, currentQuestionIndex, response)
}

/**
* Identify a user with a unique ID instead of a PostHog
* randomly generated distinct_id. If the method is never called,
Expand Down
Loading
Loading