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: Reacting to config changes #1138

Merged
merged 17 commits into from
Apr 17, 2024
69 changes: 2 additions & 67 deletions src/__tests__/decide.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { Decide } from '../decide'
import { PostHogPersistence } from '../posthog-persistence'
import { RequestRouter } from '../utils/request-router'
import { checkScriptsForSrc } from './helpers/script-utils'


const expectDecodedSendRequest = (send_request, data, noCompression) => {
const lastCall = send_request.mock.calls[send_request.mock.calls.length - 1]
Expand All @@ -19,24 +21,6 @@ const expectDecodedSendRequest = (send_request, data, noCompression) => {
})
}

const checkScriptsForSrc = (src, negate = false) => {
const scripts = document.querySelectorAll('body > script')
let foundScript = false
for (let i = 0; i < scripts.length; i++) {
if (scripts[i].src === src) {
foundScript = true
break
}
}

if (foundScript && negate) {
throw new Error(`Script with src ${src} was found when it should not have been.`)
} else if (!foundScript && !negate) {
throw new Error(`Script with src ${src} was not found when it should have been.`)
} else {
return true
}
}

describe('Decide', () => {
given('decide', () => new Decide(given.posthog))
Expand Down Expand Up @@ -255,54 +239,5 @@ describe('Decide', () => {
)
expect(checkScriptsForSrc('https://test.com/site_app/1/tokentoken/hash/', true)).toBe(true)
})

it('Make sure surveys are not loaded when decide response says no', () => {
given('decideResponse', () => ({
featureFlags: { 'test-flag': true },
surveys: false,
}))
given('config', () => ({
api_host: 'https://test.com',
token: 'testtoken',
persistence: 'memory',
}))

given.subject()
// Make sure the script is not loaded
expect(checkScriptsForSrc('https://test.com/static/surveys.js', true)).toBe(true)
})

it('Make sure surveys are loaded when decide response says so', () => {
given('decideResponse', () => ({
featureFlags: { 'test-flag': true },
surveys: true,
}))
given('config', () => ({
api_host: 'https://test.com',
token: 'testtoken',
persistence: 'memory',
}))

given.subject()
// Make sure the script is loaded
expect(checkScriptsForSrc('https://test.com/static/surveys.js')).toBe(true)
})

it('Make sure surveys are not loaded when config says no', () => {
given('decideResponse', () => ({
featureFlags: { 'test-flag': true },
surveys: true,
}))
given('config', () => ({
api_host: 'https://test.com',
token: 'testtoken',
persistence: 'memory',
disable_surveys: true,
}))

given.subject()
// Make sure the script is not loaded
expect(checkScriptsForSrc('https://test.com/static/surveys.js', true)).toBe(true)
})
})
})
18 changes: 18 additions & 0 deletions src/__tests__/helpers/script-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
export const checkScriptsForSrc = (src, negate = false) => {
const scripts = document.querySelectorAll('body > script')
let foundScript = false
for (let i = 0; i < scripts.length; i++) {
if (scripts[i].src === src) {
foundScript = true
break
}
}

if (foundScript && negate) {
throw new Error(`Script with src ${src} was found when it should not have been.`)
} else if (!foundScript && !negate) {
throw new Error(`Script with src ${src} was not found when it should have been.`)
} else {
return true
}
}
23 changes: 23 additions & 0 deletions src/__tests__/surveys.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { DecideResponse, PostHogConfig, Properties } from '../types'
import { window } from '../utils/globals'
import { RequestRouter } from '../utils/request-router'
import { assignableWindow } from '../utils/globals'
import { checkScriptsForSrc } from './helpers/script-utils'

describe('surveys', () => {
let config: PostHogConfig
Expand Down Expand Up @@ -400,4 +401,26 @@ describe('surveys', () => {
})
})
})

describe("decide response", () => {
it('should not load when decide response says no', () => {
surveys.afterDecideResponse({ surveys: false} as DecideResponse)
// Make sure the script is not loaded
expect(checkScriptsForSrc('https://test.com/static/surveys.js', true)).toBe(true)
})

it('should load when decide response says so', () => {
surveys.afterDecideResponse({ surveys: true } as DecideResponse)
// Make sure the script is loaded
expect(checkScriptsForSrc('https://test.com/static/surveys.js')).toBe(true)
})

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
expect(checkScriptsForSrc('https://test.com/static/surveys.js', true)).toBe(true)
})
})
})
15 changes: 0 additions & 15 deletions src/decide.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,22 +70,7 @@ export class Decide {

this.instance._afterDecideResponse(response)

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
const surveysGenerator = window?.extendPostHogWithSurveys

// TODO: Need to change this to be able to be more reactive
if (!this.instance.config.disable_surveys && response['surveys'] && !surveysGenerator) {
loadScript(this.instance.requestRouter.endpointFor('assets', '/static/surveys.js'), (err) => {
if (err) {
return logger.error(`Could not load surveys script`, err)
}

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
window.extendPostHogWithSurveys(this.instance)
})
}

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
Expand Down
2 changes: 2 additions & 0 deletions src/posthog-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@ export class PostHog {

this.autocapture = new Autocapture(this)
this.autocapture.startOrStopIfEnabled()
this.surveys.startOrStopIfEnabled()

// if any instance on the page has debug = true, we set the
// global debug to be true
Expand Down Expand Up @@ -1667,6 +1668,7 @@ export class PostHog {

this.sessionRecording?.startOrStopIfEnabled()
this.autocapture?.startOrStopIfEnabled()
this.surveys.startOrStopIfEnabled()
}
}

Expand Down
25 changes: 24 additions & 1 deletion src/posthog-surveys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import { PostHog } from './posthog-core'
import { SURVEYS } from './constants'
import { SurveyCallback, SurveyUrlMatchType } from './posthog-surveys-types'
import { _isUrlMatchingRegex } from './utils/request-utils'
import { window, document } from './utils/globals'
import { window, document, assignableWindow } from './utils/globals'
import { DecideResponse } from './types'
import { loadScript } from './utils'
import { logger } from './utils/logger'

export const surveyUrlValidationMap: Record<SurveyUrlMatchType, (conditionsUrl: string) => boolean> = {
icontains: (conditionsUrl) =>
Expand All @@ -13,11 +16,31 @@ export const surveyUrlValidationMap: Record<SurveyUrlMatchType, (conditionsUrl:

export class PostHogSurveys {
instance: PostHog
private _decideServerResponse?: boolean

constructor(instance: PostHog) {
this.instance = instance
}

afterDecideResponse(response: DecideResponse) {
this._decideServerResponse = !!response['surveys']
this.startOrStopIfEnabled()
}

startOrStopIfEnabled() {
const surveysGenerator = assignableWindow?.extendPostHogWithSurveys

if (!this.instance.config.disable_surveys && this._decideServerResponse && !surveysGenerator) {
loadScript(this.instance.requestRouter.endpointFor('assets', '/static/surveys.js'), (err) => {
if (err) {
return logger.error(`Could not load surveys script`, err)
}

assignableWindow.extendPostHogWithSurveys(this.instance)
})
}
}

getSurveys(callback: SurveyCallback, forceReload = false) {
// In case we manage to load the surveys script, but config says not to load surveys
// then we shouldn't return survey data
Expand Down
Loading