Skip to content

Commit

Permalink
fix: Handle uninitialised helpers better (#767)
Browse files Browse the repository at this point in the history 
* Handle init values better when unintialised

* Fix tests

* Fix

* Added logging

* Fixed
  • Loading branch information
@benjackwhite
benjackwhite authored Sep 7, 2023
1 parent 35e5992 commit 580a055
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 63 deletions.
10 changes: 5 additions & 5 deletions src/__tests__/posthog-core.identify.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ jest.mock('../gdpr-utils', () => ({
}))
jest.mock('../decide')

given('lib', () => Object.assign(new PostHog(), given.overrides))
given('lib', () => {
const posthog = new PostHog()
posthog._init('testtoken', given.config, 'testhog')
return Object.assign(posthog, given.overrides)
})

describe('identify()', () => {
given(
Expand Down Expand Up @@ -339,10 +343,6 @@ describe('reset()', () => {
persistence: new PostHogPersistence(given.config),
}))

beforeEach(() => {
given.lib._init('testtoken', given.config, 'testhog')
})

it('clears persistence', () => {
given.lib.persistence.register({ $enabled_feature_flags: { flag: 'variant', other: true } })
expect(given.lib.persistence.props['$enabled_feature_flags']).toEqual({ flag: 'variant', other: true })
Expand Down
9 changes: 8 additions & 1 deletion src/__tests__/posthog-core.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ jest.mock('../utils', () => ({
document: { title: 'test' },
}))

given('lib', () => Object.assign(new PostHog(), given.overrides))
given('lib', () => {
const posthog = new PostHog()
posthog._init('testtoken', given.config, 'testhog')
return Object.assign(posthog, given.overrides)
})

describe('capture()', () => {
given('eventName', () => '$event')
Expand All @@ -29,6 +33,7 @@ describe('capture()', () => {
given('config', () => ({
property_blacklist: [],
_onCapture: jest.fn(),
get_device_id: jest.fn().mockReturnValue('device-id'),
}))

given('overrides', () => ({
Expand All @@ -38,11 +43,13 @@ describe('capture()', () => {
persistence: {
remove_event_timer: jest.fn(),
properties: jest.fn(),
update_config: jest.fn(),
},
sessionPersistence: {
update_search_keyword: jest.fn(),
update_campaign_params: jest.fn(),
update_referrer_info: jest.fn(),
update_config: jest.fn(),
properties: jest.fn(),
},
compression: {},
Expand Down
35 changes: 26 additions & 9 deletions src/extensions/sessionrecording.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,15 @@ export class SessionRecording {
})
}

private getSessionManager() {
if (!this.instance.sessionManager) {
logger.error('Session recording started without valid sessionManager')
return
}

return this.instance.sessionManager
}

startRecordingIfEnabled() {
if (this.isRecordingEnabled()) {
this.startCaptureAndTrySendingQueuedSnapshots()
Expand Down Expand Up @@ -185,14 +194,18 @@ export class SessionRecording {
}

private _startCapture() {
// According to the rrweb docs, rrweb is not supported on IE11 and below:
// "rrweb does not support IE11 and below because it uses the MutationObserver API which was supported by these browsers."
// https://github.com/rrweb-io/rrweb/blob/master/guide.md#compatibility-note
//
// However, MutationObserver does exist on IE11, it just doesn't work well and does not detect all changes.
// Instead, when we load "recorder.js", the first JS error is about "Object.assign" being undefined.
// Thus instead of MutationObserver, we look for this function and block recording if it's undefined.
const sessionManager = this.getSessionManager()
if (!sessionManager) {
return
}
if (typeof Object.assign === 'undefined') {
// According to the rrweb docs, rrweb is not supported on IE11 and below:
// "rrweb does not support IE11 and below because it uses the MutationObserver API which was supported by these browsers."
// https://github.com/rrweb-io/rrweb/blob/master/guide.md#compatibility-note
//
// However, MutationObserver does exist on IE11, it just doesn't work well and does not detect all changes.
// Instead, when we load "recorder.js", the first JS error is about "Object.assign" being undefined.
// Thus instead of MutationObserver, we look for this function and block recording if it's undefined.
return
}

Expand All @@ -203,7 +216,7 @@ export class SessionRecording {

this.captureStarted = true
// We want to ensure the sessionManager is reset if necessary on load of the recorder
this.instance.sessionManager.checkAndGetSessionAndWindowId()
sessionManager.checkAndGetSessionAndWindowId()

const recorderJS = this.getRecordingVersion() === 'v2' ? 'recorder-v2.js' : 'recorder.js'

Expand Down Expand Up @@ -231,6 +244,10 @@ export class SessionRecording {
}

private _updateWindowAndSessionIds(event: eventWithTime) {
const sessionManager = this.getSessionManager()
if (!sessionManager) {
return
}
// Some recording events are triggered by non-user events (e.g. "X minutes ago" text updating on the screen).
// We don't want to extend the session or trigger a new session in these cases. These events are designated by event
// type -> incremental update, and source -> mutation.
Expand Down Expand Up @@ -258,7 +275,7 @@ export class SessionRecording {
}

// We only want to extend the session if it is an interactive event.
const { windowId, sessionId } = this.instance.sessionManager.checkAndGetSessionAndWindowId(
const { windowId, sessionId } = sessionManager.checkAndGetSessionAndWindowId(
!isUserInteraction,
event.timestamp
)
Expand Down
94 changes: 51 additions & 43 deletions src/posthog-core.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,20 +265,22 @@ export class PostHog {
__loaded_recorder_version: 'v1' | 'v2' | undefined // flag that keeps track of which version of recorder is loaded
config: PostHogConfig

persistence: PostHogPersistence
rateLimiter: RateLimiter
sessionPersistence: PostHogPersistence
sessionManager: SessionIdManager
pageViewManager: PageViewManager
featureFlags: PostHogFeatureFlags
surveys: PostHogSurveys
toolbar: Toolbar
sessionRecording: SessionRecording | undefined
webPerformance: WebPerformanceObserver | undefined
exceptionAutocapture: ExceptionObserver | undefined

_requestQueue: RequestQueue
_retryQueue: RetryQueue
// These are instance-specific state created after initialisation
persistence?: PostHogPersistence
sessionPersistence?: PostHogPersistence
sessionManager?: SessionIdManager

_requestQueue?: RequestQueue
_retryQueue?: RetryQueue
sessionRecording?: SessionRecording
webPerformance?: WebPerformanceObserver
exceptionAutocapture?: ExceptionObserver

_triggered_notifs: any
compression: Partial<Record<Compression, boolean>>
Expand Down Expand Up @@ -316,13 +318,6 @@ export class PostHog {
this.surveys = new PostHogSurveys(this)
this.rateLimiter = new RateLimiter()

// these are created in _init() after we have the config
this._requestQueue = undefined as any
this._retryQueue = undefined as any
this.persistence = undefined as any
this.sessionPersistence = undefined as any
this.sessionManager = undefined as any

// NOTE: See the property definition for deprecation notice
this.people = {
set: (prop: string | Properties, to?: string, callback?: RequestCallback) => {
Expand Down Expand Up @@ -559,7 +554,7 @@ export class PostHog {
_start_queue_if_opted_in(): void {
if (!this.has_opted_out_capturing()) {
if (this.get_config('request_batching')) {
this._requestQueue.poll()
this._requestQueue?.poll()
}
}
}
Expand Down Expand Up @@ -622,8 +617,8 @@ export class PostHog {
this.capture('$pageleave')
}

this._requestQueue.unload()
this._retryQueue.unload()
this._requestQueue?.unload()
this._retryQueue?.unload()
}

_handle_queued_event(url: string, data: Record<string, any>, options?: XHROptions): void {
Expand All @@ -642,6 +637,9 @@ export class PostHog {
}

_send_request(url: string, data: Record<string, any>, options: CaptureOptions, callback?: RequestCallback): void {
if (!this.__loaded || !this._retryQueue) {
return
}
if (this.rateLimiter.isRateLimited(options._batchKey)) {
if (this.get_config('debug')) {
console.warn('[PostHog SendRequest] is quota limited. Dropping request.')
Expand Down Expand Up @@ -834,8 +832,8 @@ export class PostHog {
): CaptureResult | void {
// While developing, a developer might purposefully _not_ call init(),
// in this case, we would like capture to be a noop.
if (!this.__loaded) {
return
if (!this.__loaded || !this.sessionPersistence || !this._requestQueue) {
return logger.unintializedWarning('posthog.capture')
}

if (userOptedOut(this, false)) {
Expand Down Expand Up @@ -919,6 +917,10 @@ export class PostHog {
}

_calculate_event_properties(event_name: string, event_properties: Properties): Properties {
if (!this.persistence || !this.sessionPersistence) {
return event_properties
}

// set defaults
const start_timestamp = this.persistence.remove_event_timer(event_name)
let properties = { ...event_properties }
Expand Down Expand Up @@ -1019,7 +1021,7 @@ export class PostHog {
* @param {Number} [days] How many days since the user's last visit to store the super properties
*/
register(properties: Properties, days?: number): void {
this.persistence.register(properties, days)
this.persistence?.register(properties, days)
}

/**
Expand All @@ -1046,7 +1048,7 @@ export class PostHog {
* @param {Number} [days] How many days since the users last visit to store the super properties
*/
register_once(properties: Properties, default_value?: Property, days?: number): void {
this.persistence.register_once(properties, default_value, days)
this.persistence?.register_once(properties, default_value, days)
}

/**
Expand All @@ -1073,7 +1075,7 @@ export class PostHog {
* @param {Object} properties An associative array of properties to store about the user
*/
register_for_session(properties: Properties): void {
this.sessionPersistence.register(properties)
this.sessionPersistence?.register(properties)
}

/**
Expand All @@ -1082,7 +1084,7 @@ export class PostHog {
* @param {String} property The name of the super property to remove
*/
unregister(property: string): void {
this.persistence.unregister(property)
this.persistence?.unregister(property)
}

/**
Expand All @@ -1091,7 +1093,7 @@ export class PostHog {
* @param {String} property The name of the session super property to remove
*/
unregister_for_session(property: string): void {
this.sessionPersistence.unregister(property)
this.sessionPersistence?.unregister(property)
}

_register_single(prop: string, value: Property) {
Expand Down Expand Up @@ -1191,7 +1193,7 @@ export class PostHog {
* @returns {Function} A function that can be called to unsubscribe the listener. E.g. Used by useEffect when the component unmounts.
*/
onSessionId(callback: SessionIdChangedCallback): () => void {
return this.sessionManager.onSessionId(callback)
return this.sessionManager?.onSessionId(callback) ?? (() => {})
}

/** Get list of all surveys. */
Expand Down Expand Up @@ -1251,6 +1253,9 @@ export class PostHog {
* @param {Object} [userPropertiesToSetOnce] Optional: An associative array of properties to store about the user. If property is previously set, this does not override that value.
*/
identify(new_distinct_id?: string, userPropertiesToSet?: Properties, userPropertiesToSetOnce?: Properties): void {
if (!this.__loaded || !this.persistence) {
return logger.unintializedWarning('posthog.identify')
}
//if the new_distinct_id has not been set ignore the identify event
if (!new_distinct_id) {
console.error('Unique user id has not been set in posthog.identify')
Expand Down Expand Up @@ -1417,11 +1422,14 @@ export class PostHog {
* Useful for clearing data when a user logs out.
*/
reset(reset_device_id?: boolean): void {
if (!this.__loaded) {
return logger.unintializedWarning('posthog.reset')
}
const device_id = this.get_property('$device_id')
this.persistence.clear()
this.sessionPersistence.clear()
this.persistence.set_user_state('anonymous')
this.sessionManager.resetSessionId()
this.persistence?.clear()
this.sessionPersistence?.clear()
this.persistence?.set_user_state('anonymous')
this.sessionManager?.resetSessionId()
const uuid = this.get_config('get_device_id')(uuidv7())
this.register_once(
{
Expand Down Expand Up @@ -1464,7 +1472,7 @@ export class PostHog {
*/

get_session_id(): string {
return this.sessionManager.checkAndGetSessionAndWindowId(true).sessionId
return this.sessionManager?.checkAndGetSessionAndWindowId(true).sessionId ?? ''
}

/**
Expand All @@ -1475,6 +1483,9 @@ export class PostHog {
* @param options.timestampLookBack How many seconds to look back for the timestamp (defaults to 10)
*/
get_session_replay_url(options?: { withTimestamp?: boolean; timestampLookBack?: number }): string {
if (!this.sessionManager) {
return ''
}
const host = this.config.ui_host || this.config.api_host
const { sessionId, sessionStartTimestamp } = this.sessionManager.checkAndGetSessionAndWindowId(true)
let url = host + '/replay/' + sessionId
Expand Down Expand Up @@ -1681,12 +1692,9 @@ export class PostHog {
this.config.disable_persistence = this.config.disable_cookie
}

if (this.persistence) {
this.persistence.update_config(this.config)
}
if (this.sessionPersistence) {
this.sessionPersistence.update_config(this.config)
}
this.persistence?.update_config(this.config)
this.sessionPersistence?.update_config(this.config)

if (localStore.is_supported() && localStore.get('ph_debug') === 'true') {
this.config.debug = true
}
Expand Down Expand Up @@ -1765,7 +1773,7 @@ export class PostHog {
* @param {String} property_name The name of the super property you want to retrieve
*/
get_property(property_name: string): Property | undefined {
return this.persistence['props'][property_name]
return this.persistence?.['props'][property_name]
}

/**
Expand All @@ -1788,7 +1796,7 @@ export class PostHog {
* @param {String} property_name The name of the session super property you want to retrieve
*/
getSessionProperty(property_name: string): Property | undefined {
return this.sessionPersistence['props'][property_name]
return this.sessionPersistence?.['props'][property_name]
}

toString(): string {
Expand Down Expand Up @@ -1851,11 +1859,11 @@ export class PostHog {
return
}

if (!this.get_config('disable_persistence') && this.persistence.disabled !== disabled) {
this.persistence.set_disabled(disabled)
if (!this.get_config('disable_persistence') && this.persistence?.disabled !== disabled) {
this.persistence?.set_disabled(disabled)
}
if (!this.get_config('disable_persistence') && this.sessionPersistence.disabled !== disabled) {
this.sessionPersistence.set_disabled(disabled)
if (!this.get_config('disable_persistence') && this.sessionPersistence?.disabled !== disabled) {
this.sessionPersistence?.set_disabled(disabled)
}
}

Expand Down
Loading

0 comments on commit 580a055

Please sign in to comment.