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: Window or document access across the code #894

Merged
merged 6 commits into from
Nov 15, 2023
Merged

fix: Window or document access across the code #894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4302935
Fixed up window/document access
benjackwhite Nov 15, 2023
0564a68
Fixes
benjackwhite Nov 15, 2023
358e5af
Merge branch 'master' into fix/window-access
benjackwhite Nov 15, 2023
21e4127
Fix
benjackwhite Nov 15, 2023
4dc6e5f
Merge branch 'master' into fix/window-access
benjackwhite Nov 15, 2023
4d23f2d
fix: Allow window to be truly undefined (#895)
benjackwhite Nov 15, 2023
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
8 changes: 8 additions & 0 deletions .eslintrc.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading