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: survey preview better bundle size #1665

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
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
30 changes: 16 additions & 14 deletions src/__tests__/surveys.test.ts
Original file line number Diff line number Diff line change
@@ -1,27 +1,26 @@
/// <reference lib="dom" />

import { PostHogSurveys } from '../posthog-surveys'
import {
SurveyType,
SurveyQuestionType,
Survey,
MultipleSurveyQuestion,
SurveyQuestionBranchingType,
SurveyQuestion,
RatingSurveyQuestion,
} from '../posthog-surveys-types'
import { generateSurveys, getNextSurveyStep } from '../extensions/surveys'
import {
canActivateRepeatedly,
getDisplayOrderChoices,
getDisplayOrderQuestions,
} from '../extensions/surveys/surveys-utils'
import { PostHogPersistence } from '../posthog-persistence'
import { PostHog } from '../posthog-core'
import { PostHogPersistence } from '../posthog-persistence'
import { PostHogSurveys } from '../posthog-surveys'
import {
MultipleSurveyQuestion,
RatingSurveyQuestion,
Survey,
SurveyQuestion,
SurveyQuestionBranchingType,
SurveyQuestionType,
SurveyType,
} from '../posthog-surveys-types'
import { DecideResponse, PostHogConfig, Properties } from '../types'
import { window } from '../utils/globals'
import { assignableWindow, window } from '../utils/globals'
import { RequestRouter } from '../utils/request-router'
import { assignableWindow } from '../utils/globals'
import { generateSurveys } from '../extensions/surveys'

describe('surveys', () => {
let config: PostHogConfig
Expand Down Expand Up @@ -844,6 +843,9 @@ describe('surveys', () => {
})

describe('branching logic', () => {
beforeEach(() => {
surveys.getNextSurveyStep = getNextSurveyStep
Copy link
Member

Choose a reason for hiding this comment

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

i tend to mock the external dependency loader so that more of the flow is being exercised...

so e.g. in session replay

        loadScriptMock.mockImplementation((_ph, _path, callback) => {
            addRRwebToWindow()
            callback()
        })

        assignableWindow.__PosthogExtensions__.loadExternalDependency = loadScriptMock
        ```
        
        otherwise that startup can be broken and tests carry on passing
        
        

Copy link
Member

Choose a reason for hiding this comment

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

ofc explicit tests somewhere else for it is totally fine too

})
const survey: Survey = {
name: 'My survey',
description: '',
Expand Down
3 changes: 1 addition & 2 deletions src/entrypoints/surveys-preview.es.ts
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'
5 changes: 3 additions & 2 deletions src/entrypoints/surveys.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import { generateSurveys } from '../extensions/surveys'
import { generateSurveys, getNextSurveyStep } from '../extensions/surveys'

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

assignableWindow.__PosthogExtensions__ = assignableWindow.__PosthogExtensions__ || {}
assignableWindow.__PosthogExtensions__.canActivateRepeatedly = canActivateRepeatedly
assignableWindow.__PosthogExtensions__.generateSurveys = generateSurveys
assignableWindow.__PosthogExtensions__.getNextSurveyStep = getNextSurveyStep

// this used to be directly on window, but we moved it to __PosthogExtensions__
// it is still on window for backwards compatibility
Expand Down
103 changes: 103 additions & 0 deletions src/extensions/surveys.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -759,3 +759,106 @@ const getQuestionComponent = ({

return <Component {...componentProps} />
}

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
}
119 changes: 8 additions & 111 deletions src/posthog-surveys.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
import { SURVEYS } from './constants'
import { getSurveySeenStorageKeys } from './extensions/surveys/surveys-utils'
import { PostHog } from './posthog-core'
import {
Survey,
SurveyCallback,
SurveyQuestionBranchingType,
SurveyQuestionType,
SurveyUrlMatchType,
} from './posthog-surveys-types'
import { Survey, SurveyCallback, SurveyUrlMatchType } from './posthog-surveys-types'
import { RemoteConfig } from './types'
import { assignableWindow, document, window } from './utils/globals'
import { createLogger } from './utils/logger'
Expand All @@ -28,109 +22,6 @@ export const surveyUrlValidationMap: Record<SurveyUrlMatchType, (conditionsUrl:
is_not: (conditionsUrl) => window?.location.href !== conditionsUrl,
}

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 PostHogSurveys {
private _decideServerResponse?: boolean
public _surveyEventReceiver: SurveyEventReceiver | null
Expand Down Expand Up @@ -302,7 +193,13 @@ export class PostHogSurveys {
return this.instance.featureFlags.isFeatureEnabled(value)
})
}
getNextSurveyStep = getNextSurveyStep
getNextSurveyStep(survey: Survey, currentQuestionIndex: number, response: string | string[] | number | null) {
if (isNullish(assignableWindow.__PosthogExtensions__?.getNextSurveyStep)) {
logger.warn('init was not called')
return 0
}
Comment on lines +197 to +200
Copy link
Member

Choose a reason for hiding this comment

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

Lets do better logging here similar to #1663
so we know if the problem is the PosthogExtensions not loaded, or the getNextSurveyStep somehow wasn't available

return assignableWindow.__PosthogExtensions__.getNextSurveyStep(survey, currentQuestionIndex, response)
}

// this method is lazily loaded onto the window to avoid loading preact and other dependencies if surveys is not enabled
private _canActivateRepeatedly(survey: Survey) {
Expand Down
1 change: 1 addition & 0 deletions src/utils/globals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ interface PostHogExtensions {
rrwebPlugins?: { getRecordConsolePlugin: any; getRecordNetworkPlugin?: any }
canActivateRepeatedly?: (survey: any) => boolean
generateSurveys?: (posthog: PostHog) => any | undefined
getNextSurveyStep?: (survey: any, currentQuestionIndex: number, response: string | string[] | number | null) => any
Copy link
Member

Choose a reason for hiding this comment

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

we can't move generateSurveys because older clients are already reading from that location...

but it might be nice to put a surveys on here so you'd have

// deprecated: still in place for older clients but you should use `surveys.generateSurveys`
generateSurveys?: (posthog: PostHog) => any | undefined
surveys: {
	generateSurveys?: (posthog: PostHog) => any | undefined,
    getNextSurveyStep?: (survey: any, currentQuestionIndex: number, response: string | string[] | number | null) => 
any
}

so that over time it stays obvious what surveys has put on the window

not a hard requirement but i think tidier in the long run (and i wish we'd started doing this earlier)

postHogWebVitalsCallbacks?: {
onLCP: (metric: any) => void
onCLS: (metric: any) => void
Expand Down
Loading