Skip to content

Commit

Permalink
fix: Window or document access across the code (#894)
Browse files Browse the repository at this point in the history
  • Loading branch information
@benjackwhite
benjackwhite authored Nov 15, 2023
1 parent 6c8af6d commit 62c96e7
Show file tree
Hide file tree
Showing 30 changed files with 239 additions and 167 deletions.
8 changes: 8 additions & 0 deletions .eslintrc.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ module.exports = {
},
},
overrides: [
{
files: 'src/**/*',
rules: {
...rules,
'no-restricted-globals': ['error', 'document', 'window'],
},
},
{
files: 'src/__tests__/**/*',
// the same set of config as in the root
Expand All @@ -62,6 +69,7 @@ module.exports = {
rules: {
...rules,
'no-console': 'off',
'no-restricted-globals': 'off',
},
},
{
Expand Down
53 changes: 27 additions & 26 deletions src/__tests__/extensions/replay/sessionrecording.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
RECORDING_MAX_EVENT_SIZE,
SessionRecording,
} from '../../../extensions/replay/sessionrecording'
import { assignableWindow } from '../../../utils/globals'

// Type and source defined here designate a non-user-generated recording event

Expand Down Expand Up @@ -52,8 +53,8 @@ describe('SessionRecording', () => {
let onFeatureFlagsCallback: ((flags: string[]) => void) | null

beforeEach(() => {
;(window as any).rrwebRecord = jest.fn()
;(window as any).rrwebConsoleRecord = {
assignableWindow.rrwebRecord = jest.fn()
assignableWindow.rrwebConsoleRecord = {
getRecordConsolePlugin: jest.fn(),
}

Expand Down Expand Up @@ -193,7 +194,7 @@ describe('SessionRecording', () => {
beforeEach(() => {
jest.spyOn(sessionRecording, 'startRecordingIfEnabled')
;(loadScript as any).mockImplementation((_path: any, callback: any) => callback())
;(window as any).rrwebRecord = jest.fn(({ emit }) => {
assignableWindow.rrwebRecord = jest.fn(({ emit }) => {
_emit = emit
return () => {}
})
Expand Down Expand Up @@ -277,11 +278,11 @@ describe('SessionRecording', () => {
describe('recording', () => {
beforeEach(() => {
const mockFullSnapshot = jest.fn()
;(window as any).rrwebRecord = jest.fn(({ emit }) => {
assignableWindow.rrwebRecord = jest.fn(({ emit }) => {
_emit = emit
return () => {}
})
;(window as any).rrwebRecord.takeFullSnapshot = mockFullSnapshot
assignableWindow.rrwebRecord.takeFullSnapshot = mockFullSnapshot
;(loadScript as any).mockImplementation((_path: any, callback: any) => callback())
})

Expand Down Expand Up @@ -356,7 +357,7 @@ describe('SessionRecording', () => {
sessionRecording: { endpoint: '/s/', sampleRate: '0.50' },
})
)
const emitValues = []
const emitValues: string[] = []
let lastSessionId = sessionRecording['sessionId']

for (let i = 0; i < 100; i++) {
Expand All @@ -368,7 +369,7 @@ describe('SessionRecording', () => {
expect(sessionRecording['sessionId']).not.toBe(lastSessionId)
lastSessionId = sessionRecording['sessionId']

emitValues.push(sessionRecording['status'])
emitValues.push(sessionRecording.status)
}

// the random number generator won't always be exactly 0.5, but it should be close
Expand All @@ -384,7 +385,7 @@ describe('SessionRecording', () => {

// maskAllInputs should change from default
// someUnregisteredProp should not be present
expect((window as any).rrwebRecord).toHaveBeenCalledWith({
expect(assignableWindow.rrwebRecord).toHaveBeenCalledWith({
emit: expect.anything(),
maskAllInputs: false,
blockClass: 'ph-no-capture',
Expand Down Expand Up @@ -680,15 +681,15 @@ describe('SessionRecording', () => {

sessionRecording.startRecordingIfEnabled()

expect((window as any).rrwebConsoleRecord.getRecordConsolePlugin).not.toHaveBeenCalled()
expect(assignableWindow.rrwebConsoleRecord.getRecordConsolePlugin).not.toHaveBeenCalled()
})

it('if enabled, plugin is used', () => {
posthog.config.enable_recording_console_log = true

sessionRecording.startRecordingIfEnabled()

expect((window as any).rrwebConsoleRecord.getRecordConsolePlugin).toHaveBeenCalled()
expect(assignableWindow.rrwebConsoleRecord.getRecordConsolePlugin).toHaveBeenCalled()
})
})

Expand All @@ -710,32 +711,32 @@ describe('SessionRecording', () => {
sessionIdGeneratorMock.mockImplementation(() => 'newSessionId')
windowIdGeneratorMock.mockImplementation(() => 'newWindowId')
_emit(createIncrementalSnapshot())
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalled()
expect(assignableWindow.rrwebRecord.takeFullSnapshot).toHaveBeenCalled()
})

it('sends a full snapshot if there is a new window id and the event is not type FullSnapshot or Meta', () => {
sessionIdGeneratorMock.mockImplementation(() => 'old-session-id')
windowIdGeneratorMock.mockImplementation(() => 'newWindowId')
_emit(createIncrementalSnapshot())
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalled()
expect(assignableWindow.rrwebRecord.takeFullSnapshot).toHaveBeenCalled()
})

it('does not send a full snapshot if there is a new session/window id and the event is type FullSnapshot or Meta', () => {
sessionIdGeneratorMock.mockImplementation(() => 'newSessionId')
windowIdGeneratorMock.mockImplementation(() => 'newWindowId')
_emit(createIncrementalSnapshot({ type: META_EVENT_TYPE }))
expect((window as any).rrwebRecord.takeFullSnapshot).not.toHaveBeenCalled()
expect(assignableWindow.rrwebRecord.takeFullSnapshot).not.toHaveBeenCalled()
})

it('does not send a full snapshot if there is not a new session or window id', () => {
;(window as any).rrwebRecord.takeFullSnapshot.mockClear()
assignableWindow.rrwebRecord.takeFullSnapshot.mockClear()

sessionIdGeneratorMock.mockImplementation(() => 'old-session-id')
windowIdGeneratorMock.mockImplementation(() => 'old-window-id')
sessionManager.resetSessionId()

_emit(createIncrementalSnapshot())
expect((window as any).rrwebRecord.takeFullSnapshot).not.toHaveBeenCalled()
expect(assignableWindow.rrwebRecord.takeFullSnapshot).not.toHaveBeenCalled()
})
})

Expand Down Expand Up @@ -843,7 +844,7 @@ describe('SessionRecording', () => {

it('takes a full snapshot for the first _emit', () => {
emitAtDateTime(startingDate)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)
expect(assignableWindow.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)
})

it('does not take a full snapshot for the second _emit', () => {
Expand All @@ -857,7 +858,7 @@ describe('SessionRecording', () => {
startingDate.getMinutes() + 1
)
)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)
expect(assignableWindow.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)
})

it('does not change session id for a second _emit', () => {
Expand Down Expand Up @@ -899,7 +900,7 @@ describe('SessionRecording', () => {
startingDate.getMinutes() + 2
)
)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)
expect(assignableWindow.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)
})

it('sends a full snapshot if the session is rotated because session has been inactive for 30 minutes', () => {
Expand All @@ -925,7 +926,7 @@ describe('SessionRecording', () => {
emitAtDateTime(inactivityThresholdLater)

expect(sessionManager['_getSessionId']()[1]).not.toEqual(startingSessionId)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(2)
expect(assignableWindow.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(2)
})

it('sends a full snapshot if the session is rotated because max time has passed', () => {
Expand All @@ -950,7 +951,7 @@ describe('SessionRecording', () => {
emitAtDateTime(moreThanADayLater)

expect(sessionManager['_getSessionId']()[1]).not.toEqual(startingSessionId)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(2)
expect(assignableWindow.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(2)
})
})

Expand All @@ -960,7 +961,7 @@ describe('SessionRecording', () => {
const lastActivityTimestamp = sessionRecording['_lastActivityTimestamp']
expect(lastActivityTimestamp).toBeGreaterThan(0)

expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0)
expect(assignableWindow.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(0)

_emit({
event: 123,
Expand All @@ -973,7 +974,7 @@ describe('SessionRecording', () => {
expect(sessionRecording['isIdle']).toEqual(false)
expect(sessionRecording['_lastActivityTimestamp']).toEqual(lastActivityTimestamp + 100)

expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)
expect(assignableWindow.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)

_emit({
event: 123,
Expand All @@ -985,7 +986,7 @@ describe('SessionRecording', () => {
})
expect(sessionRecording['isIdle']).toEqual(false)
expect(sessionRecording['_lastActivityTimestamp']).toEqual(lastActivityTimestamp + 100)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)
expect(assignableWindow.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)

// this triggers idle state and isn't a user interaction so does not take a full snapshot
_emit({
Expand All @@ -998,7 +999,7 @@ describe('SessionRecording', () => {
})
expect(sessionRecording['isIdle']).toEqual(true)
expect(sessionRecording['_lastActivityTimestamp']).toEqual(lastActivityTimestamp + 100)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)
expect(assignableWindow.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(1)

// this triggers idle state _and_ is a user interaction, so we take a full snapshot
_emit({
Expand All @@ -1013,7 +1014,7 @@ describe('SessionRecording', () => {
expect(sessionRecording['_lastActivityTimestamp']).toEqual(
lastActivityTimestamp + RECORDING_IDLE_ACTIVITY_TIMEOUT_MS + 2000
)
expect((window as any).rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(2)
expect(assignableWindow.rrwebRecord.takeFullSnapshot).toHaveBeenCalledTimes(2)
})
})
})
Expand Down Expand Up @@ -1046,7 +1047,7 @@ describe('SessionRecording', () => {

describe('buffering minimum duration', () => {
beforeEach(() => {
;(window as any).rrwebRecord = jest.fn(({ emit }) => {
assignableWindow.rrwebRecord = jest.fn(({ emit }) => {
_emit = emit
return () => {}
})
Expand Down
31 changes: 16 additions & 15 deletions src/__tests__/extensions/toolbar.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Toolbar } from '../../extensions/toolbar'
import { _isString, _isUndefined } from '../../utils/type-utils'
import { PostHog } from '../../posthog-core'
import { PostHogConfig, ToolbarParams } from '../../types'
import { window } from '../../utils/globals'
import { assignableWindow, window } from '../../utils/globals'

jest.mock('../../utils', () => ({
...jest.requireActual('../../utils'),
Expand Down Expand Up @@ -31,16 +31,17 @@ describe('Toolbar', () => {
})

beforeEach(() => {
;(window as any).ph_load_toolbar = jest.fn()
delete (window as any)['_postHogToolbarLoaded']
assignableWindow.ph_load_toolbar = jest.fn()
delete assignableWindow['_postHogToolbarLoaded']
})

describe('maybeLoadToolbar', () => {
const localStorage = {
getItem: jest.fn(),
setItem: jest.fn(),
}
const history = { replaceState: jest.fn() }
const storage = localStorage as unknown as Storage
const history = { replaceState: jest.fn() } as unknown as History

const defaultHashState = {
action: 'ph_authorize',
Expand Down Expand Up @@ -71,7 +72,7 @@ describe('Toolbar', () => {
.join('&')
}

const aLocation = (hash?: string) => {
const aLocation = (hash?: string): Location => {
if (_isUndefined(hash)) {
hash = withHash(withHashParamsFrom())
}
Expand All @@ -80,7 +81,7 @@ describe('Toolbar', () => {
hash: `#${hash}`,
pathname: 'pathname',
search: '?search',
}
} as Location
}

beforeEach(() => {
Expand All @@ -91,7 +92,7 @@ describe('Toolbar', () => {

it('should initialize the toolbar when the hash state contains action "ph_authorize"', () => {
// the default hash state in the test setup contains the action "ph_authorize"
toolbar.maybeLoadToolbar(aLocation(), localStorage, history)
toolbar.maybeLoadToolbar(aLocation(), storage, history)

expect(toolbar.loadToolbar).toHaveBeenCalledWith({
...toolbarParams,
Expand All @@ -105,7 +106,7 @@ describe('Toolbar', () => {
localStorage.getItem.mockImplementation(() => JSON.stringify(toolbarParams))

const hashState = { ...defaultHashState, action: undefined }
toolbar.maybeLoadToolbar(aLocation(withHash(withHashParamsFrom(hashState))), localStorage, history)
toolbar.maybeLoadToolbar(aLocation(withHash(withHashParamsFrom(hashState))), storage, history)

expect(toolbar.loadToolbar).toHaveBeenCalledWith({
...toolbarParams,
Expand All @@ -114,22 +115,22 @@ describe('Toolbar', () => {
})

it('should NOT initialize the toolbar when the activation query param does not exist', () => {
expect(toolbar.maybeLoadToolbar(aLocation(''), localStorage, history)).toEqual(false)
expect(toolbar.maybeLoadToolbar(aLocation(''), storage, history)).toEqual(false)

expect(toolbar.loadToolbar).not.toHaveBeenCalled()
})

it('should return false when parsing invalid JSON from fragment state', () => {
expect(
toolbar.maybeLoadToolbar(aLocation(withHash(withHashParamsFrom('literally'))), localStorage, history)
toolbar.maybeLoadToolbar(aLocation(withHash(withHashParamsFrom('literally'))), storage, history)
).toEqual(false)
expect(toolbar.loadToolbar).not.toHaveBeenCalled()
})

it('should work if calling toolbar params `__posthog`', () => {
toolbar.maybeLoadToolbar(
aLocation(withHash(withHashParamsFrom(defaultHashState, '__posthog'))),
localStorage,
storage,
history
)
expect(toolbar.loadToolbar).toHaveBeenCalledWith({ ...toolbarParams, ...defaultHashState, source: 'url' })
Expand All @@ -138,7 +139,7 @@ describe('Toolbar', () => {
it('should use the apiURL in the hash if available', () => {
toolbar.maybeLoadToolbar(
aLocation(withHash(withHashParamsFrom({ ...defaultHashState, apiURL: 'blabla' }))),
localStorage,
storage,
history
)

Expand All @@ -154,15 +155,15 @@ describe('Toolbar', () => {
describe('load and close toolbar', () => {
it('should persist for next time', () => {
expect(toolbar.loadToolbar(toolbarParams)).toBe(true)
expect(JSON.parse((window as any).localStorage.getItem('_postHogToolbarParams'))).toEqual({
expect(JSON.parse(window.localStorage.getItem('_postHogToolbarParams') ?? '')).toEqual({
...toolbarParams,
apiURL: 'http://api.example.com',
})
})

it('should load if not previously loaded', () => {
expect(toolbar.loadToolbar(toolbarParams)).toBe(true)
expect((window as any).ph_load_toolbar).toHaveBeenCalledWith(
expect(assignableWindow.ph_load_toolbar).toHaveBeenCalledWith(
{ ...toolbarParams, apiURL: 'http://api.example.com' },
instance
)
Expand All @@ -181,7 +182,7 @@ describe('Toolbar', () => {

it('should load if not previously loaded', () => {
expect(toolbar.loadToolbar(minimalToolbarParams)).toBe(true)
expect((window as any).ph_load_toolbar).toHaveBeenCalledWith(
expect(assignableWindow.ph_load_toolbar).toHaveBeenCalledWith(
{
...minimalToolbarParams,
apiURL: 'http://api.example.com',
Expand Down
4 changes: 3 additions & 1 deletion src/__tests__/storage.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ describe('sessionStore', () => {
expected: '',
},
])(`%s subdomain check`, ({ candidate, expected }) => {
expect(seekFirstNonPublicSubDomain(candidate, mockDocumentDotCookie)).toEqual(expected)
expect(seekFirstNonPublicSubDomain(candidate, mockDocumentDotCookie as unknown as Document)).toEqual(
expected
)
})
})

Expand Down
2 changes: 0 additions & 2 deletions src/__tests__/utils/event-utils.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { _info } from '../../utils/event-utils'
import * as globals from '../../utils/globals'

jest.mock('../../utils/globals')

describe(`event-utils`, () => {
describe('properties', () => {
it('should have $host and $pathname in properties', () => {
Expand Down
Loading

0 comments on commit 62c96e7

Please sign in to comment.