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: Load RemoteConfig by default but don't use it #1607

Merged
merged 3 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
4 changes: 4 additions & 0 deletions src/__tests__/helpers/posthog-instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import { PostHog, init_as_module } from '../../posthog-core'
import { PostHogConfig } from '../../types'
import { assignableWindow } from '../../utils/globals'
import { uuidv7 } from '../../uuidv7'

export const createPosthogInstance = async (
Expand All @@ -14,6 +15,9 @@ export const createPosthogInstance = async (
// creates another instance.
const posthog = new PostHog()

// NOTE: Temporary change whilst testing remote config
assignableWindow._POSTHOG_CONFIG = {} as any

// eslint-disable-next-line compat/compat
return await new Promise<PostHog>((resolve) =>
posthog.init(
Expand Down
5 changes: 4 additions & 1 deletion src/__tests__/loader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,15 @@ import { PostHog } from '../posthog-core'
import { defaultPostHog } from './helpers/posthog-instance'

import sinon from 'sinon'
import { window } from '../utils/globals'
import { assignableWindow, window } from '../utils/globals'

describe(`Module-based loader in Node env`, () => {
const posthog = defaultPostHog()

beforeEach(() => {
// NOTE: Temporary change whilst testing remote config
assignableWindow._POSTHOG_CONFIG = {} as any

jest.useFakeTimers()
jest.spyOn(posthog, '_send_request').mockReturnValue()
jest.spyOn(window!.console, 'log').mockImplementation()
Expand Down
4 changes: 4 additions & 0 deletions src/__tests__/posthog-core.identify.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@ import { USER_STATE } from '../constants'
import { uuidv7 } from '../uuidv7'
import { createPosthogInstance, defaultPostHog } from './helpers/posthog-instance'
import { PostHog } from '../posthog-core'
import { assignableWindow } from '../utils/globals'

describe('identify()', () => {
let instance: PostHog
let beforeSendMock: jest.Mock

beforeEach(() => {
beforeSendMock = jest.fn().mockImplementation((e) => e)
// NOTE: Temporary change whilst testing remote config
assignableWindow._POSTHOG_CONFIG = {} as any

const posthog = defaultPostHog().init(
uuidv7(),
{
Expand Down
2 changes: 2 additions & 0 deletions src/__tests__/posthog-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ describe('posthog core', () => {
}

beforeEach(() => {
// NOTE: Temporary change whilst testing remote config
globals.assignableWindow._POSTHOG_CONFIG = {} as any
jest.useFakeTimers().setSystemTime(baseUTCDateTime)
})

Expand Down
3 changes: 3 additions & 0 deletions src/__tests__/segment.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { beforeEach, describe, expect, it, jest } from '@jest/globals'
import { PostHog } from '../posthog-core'
import { SegmentContext, SegmentPlugin } from '../extensions/segment-integration'
import { USER_STATE } from '../constants'
import { assignableWindow } from '../utils/globals'

describe(`Segment integration`, () => {
let segment: any
Expand All @@ -21,6 +22,8 @@ describe(`Segment integration`, () => {
jest.setTimeout(500)

beforeEach(() => {
assignableWindow._POSTHOG_CONFIG = {} as any

// Create something that looks like the Segment Analytics 2.0 API. We
// could use the actual client, but it's a little more tricky and we'd
// want to mock out the network requests, for which we don't have a good
Expand Down
62 changes: 32 additions & 30 deletions src/remote-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,40 +31,36 @@ export class RemoteConfigLoader {
}

load(): void {
// Call decide to get what features are enabled and other settings.
// As a reminder, if the /decide endpoint is disabled, feature flags, toolbar, session recording, autocapture,
// and compression will not be available.

if (!this.instance.config.__preview_remote_config) {
return
}

// Attempt 1 - use the pre-loaded config if it came as part of the token-specific array.js
if (assignableWindow._POSTHOG_CONFIG) {
logger.info('Using preloaded remote config', assignableWindow._POSTHOG_CONFIG)
this.onRemoteConfig(assignableWindow._POSTHOG_CONFIG)
return
}

if (this.instance.config.advanced_disable_decide) {
// This setting is essentially saying "dont call external APIs" hence we respect it here
logger.warn('Remote config is disabled. Falling back to local config.')
return
}
try {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

diff is just the try catch and moving the "preview" check into the callback function

// Attempt 1 - use the pre-loaded config if it came as part of the token-specific array.js
if (assignableWindow._POSTHOG_CONFIG) {
logger.info('Using preloaded remote config', assignableWindow._POSTHOG_CONFIG)
this.onRemoteConfig(assignableWindow._POSTHOG_CONFIG)
return
}

// Attempt 2 - if we have the external deps loader then lets load the script version of the config that includes site apps
this._loadRemoteConfigJs((config) => {
if (!config) {
logger.info('No config found after loading remote JS config. Falling back to JSON.')
// Attempt 3 Load the config json instead of the script - we won't get site apps etc. but we will get the config
this._loadRemoteConfigJSON((config) => {
this.onRemoteConfig(config)
})
if (this.instance.config.advanced_disable_decide) {
// This setting is essentially saying "dont call external APIs" hence we respect it here
logger.warn('Remote config is disabled. Falling back to local config.')
return
}

this.onRemoteConfig(config)
})
// Attempt 2 - if we have the external deps loader then lets load the script version of the config that includes site apps
this._loadRemoteConfigJs((config) => {
if (!config) {
logger.info('No config found after loading remote JS config. Falling back to JSON.')
// Attempt 3 Load the config json instead of the script - we won't get site apps etc. but we will get the config
this._loadRemoteConfigJSON((config) => {
this.onRemoteConfig(config)
})
return
}

this.onRemoteConfig(config)
})
} catch (error) {
logger.error('Error loading remote config', error)
}
}

private onRemoteConfig(config?: RemoteConfig): void {
Expand All @@ -73,6 +69,12 @@ export class RemoteConfigLoader {
logger.error('Failed to fetch remote config from PostHog.')
return
}

if (!this.instance.config.__preview_remote_config) {
logger.info('__preview_remote_config is disabled. Logging config instead', config)
return
}

this.instance._onRemoteConfig(config)

// We only need to reload if we haven't already loaded the flags or if the request is in flight
Expand Down
Loading