From a2850a34491258be662f8336b726053703b5a44d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 11 Mar 2025 15:56:07 +0100 Subject: [PATCH 1/3] fix(browser): Only patch available `window.history` properties --- .../browser-utils/src/instrument/history.ts | 13 ++++- .../test/instrument/history.test.ts | 58 +++++++++++++++++++ packages/core/src/utils-hoist/supports.ts | 2 +- .../core/test/utils-hoist/supports.test.ts | 32 ++++++++++ 4 files changed, 101 insertions(+), 4 deletions(-) create mode 100644 packages/browser-utils/test/instrument/history.test.ts create mode 100644 packages/core/test/utils-hoist/supports.test.ts diff --git a/packages/browser-utils/src/instrument/history.ts b/packages/browser-utils/src/instrument/history.ts index 8494fca3076e..13fd4042b81e 100644 --- a/packages/browser-utils/src/instrument/history.ts +++ b/packages/browser-utils/src/instrument/history.ts @@ -18,7 +18,10 @@ export function addHistoryInstrumentationHandler(handler: (data: HandlerDataHist maybeInstrument(type, instrumentHistory); } -function instrumentHistory(): void { +/** + * Exported just for testing + */ +export function instrumentHistory(): void { // The `popstate` event may also be triggered on `pushState`, but it may not always reliably be emitted by the browser // Which is why we also monkey-patch methods below, in addition to this WINDOW.addEventListener('popstate', () => { @@ -61,6 +64,10 @@ function instrumentHistory(): void { }; } - fill(WINDOW.history, 'pushState', historyReplacementFunction); - fill(WINDOW.history, 'replaceState', historyReplacementFunction); + if (typeof WINDOW.history.pushState === 'function') { + fill(WINDOW.history, 'pushState', historyReplacementFunction); + } + if (typeof WINDOW.history.replaceState === 'function') { + fill(WINDOW.history, 'replaceState', historyReplacementFunction); + } } diff --git a/packages/browser-utils/test/instrument/history.test.ts b/packages/browser-utils/test/instrument/history.test.ts new file mode 100644 index 000000000000..9d9ffd5b0686 --- /dev/null +++ b/packages/browser-utils/test/instrument/history.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it, vi } from 'vitest'; +import { WINDOW } from '../../src/types'; +import { afterEach } from 'node:test'; + +import { instrumentHistory } from './../../src/instrument/history'; + +describe('instrumentHistory', () => { + const originalHistory = WINDOW.history; + WINDOW.addEventListener = vi.fn(); + + afterEach(() => { + // @ts-expect-error - this is fine for testing + WINDOW.history = originalHistory; + }); + + it("doesn't throw if history is not available", () => { + // @ts-expect-error - this is fine for testing + WINDOW.history = undefined; + expect(instrumentHistory).not.toThrow(); + expect(WINDOW.history).toBe(undefined); + }); + + it('adds an event listener for popstate', () => { + // adds an event listener for popstate + expect(WINDOW.addEventListener).toHaveBeenCalledTimes(1); + expect(WINDOW.addEventListener).toHaveBeenCalledWith('popstate', expect.any(Function)); + }); + + it("doesn't throw if history.pushState is not a function", () => { + // @ts-expect-error - only partially adding history properties + WINDOW.history = { + replaceState: () => {}, + pushState: undefined, + }; + + expect(instrumentHistory).not.toThrow(); + + expect(WINDOW.history).toEqual({ + replaceState: expect.any(Function), // patched function + pushState: undefined, // unpatched + }); + }); + + it("doesn't throw if history.replaceState is not a function", () => { + // @ts-expect-error - only partially adding history properties + WINDOW.history = { + replaceState: undefined, + pushState: () => {}, + }; + + expect(instrumentHistory).not.toThrow(); + + expect(WINDOW.history).toEqual({ + replaceState: undefined, // unpatched + pushState: expect.any(Function), // patched function + }); + }); +}); diff --git a/packages/core/src/utils-hoist/supports.ts b/packages/core/src/utils-hoist/supports.ts index d52c06e702ea..e486d672a625 100644 --- a/packages/core/src/utils-hoist/supports.ts +++ b/packages/core/src/utils-hoist/supports.ts @@ -61,7 +61,7 @@ export function supportsDOMException(): boolean { * @returns Answer to the given question. */ export function supportsHistory(): boolean { - return 'history' in WINDOW; + return 'history' in WINDOW && !!WINDOW.history; } /** diff --git a/packages/core/test/utils-hoist/supports.test.ts b/packages/core/test/utils-hoist/supports.test.ts new file mode 100644 index 000000000000..b2b444ab9b99 --- /dev/null +++ b/packages/core/test/utils-hoist/supports.test.ts @@ -0,0 +1,32 @@ +import { afterEach } from 'node:test'; +import { supportsHistory } from '../../src/utils-hoist/supports'; +import { describe, it, expect } from 'vitest'; + +describe('supportsHistory', () => { + const originalHistory = globalThis.history; + + afterEach(() => { + globalThis.history = originalHistory; + }); + + it('returns true if history is available', () => { + // @ts-expect-error - not setting all history properties + globalThis.history = { + pushState: () => {}, + replaceState: () => {}, + }; + expect(supportsHistory()).toBe(true); + }); + + it('returns false if history is not available', () => { + // @ts-expect-error - deletion is okay in this case + delete globalThis.history; + expect(supportsHistory()).toBe(false); + }); + + it('returns false if history is undefined', () => { + // @ts-expect-error - undefined is okay in this case + globalThis.history = undefined; + expect(supportsHistory()).toBe(false); + }); +}); From 9c77258ac6c06d61ce41e0cb40d6fdb7dcf967d4 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 11 Mar 2025 16:17:46 +0100 Subject: [PATCH 2/3] fix(core): Ensure `fill` only patches functions --- packages/browser-utils/src/instrument/history.ts | 8 ++------ packages/core/src/utils-hoist/object.ts | 8 +++++++- packages/core/test/utils-hoist/object.test.ts | 13 +++++++++++++ 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/packages/browser-utils/src/instrument/history.ts b/packages/browser-utils/src/instrument/history.ts index 13fd4042b81e..60ee888aae24 100644 --- a/packages/browser-utils/src/instrument/history.ts +++ b/packages/browser-utils/src/instrument/history.ts @@ -64,10 +64,6 @@ export function instrumentHistory(): void { }; } - if (typeof WINDOW.history.pushState === 'function') { - fill(WINDOW.history, 'pushState', historyReplacementFunction); - } - if (typeof WINDOW.history.replaceState === 'function') { - fill(WINDOW.history, 'replaceState', historyReplacementFunction); - } + fill(WINDOW.history, 'pushState', historyReplacementFunction); + fill(WINDOW.history, 'replaceState', historyReplacementFunction); } diff --git a/packages/core/src/utils-hoist/object.ts b/packages/core/src/utils-hoist/object.ts index 31d1d862d01f..3c2b04129e9e 100644 --- a/packages/core/src/utils-hoist/object.ts +++ b/packages/core/src/utils-hoist/object.ts @@ -10,6 +10,8 @@ import { truncate } from './string'; /** * Replace a method in an object with a wrapped version of itself. * + * If the method on the passed object is not a function, the wrapper will not be applied. + * * @param source An object that contains a method to be wrapped. * @param name The name of the method to be wrapped. * @param replacementFactory A higher-order function that takes the original version of the given method and returns a @@ -23,7 +25,11 @@ export function fill(source: { [key: string]: any }, name: string, replacementFa return; } - const original = source[name] as () => any; + const original = source[name]; + if (typeof original !== 'function') { + return; + } + const wrapped = replacementFactory(original) as WrappedFunction; // Make sure it's a function first, as we need to attach an empty prototype for `defineProperties` to work diff --git a/packages/core/test/utils-hoist/object.test.ts b/packages/core/test/utils-hoist/object.test.ts index 668d071108f9..f620becbf451 100644 --- a/packages/core/test/utils-hoist/object.test.ts +++ b/packages/core/test/utils-hoist/object.test.ts @@ -54,6 +54,19 @@ describe('fill()', () => { expect(source.prop()).toEqual(41); }); + test.each([42, null, undefined, {}])("does't throw if the property is not a function but %s", (propValue: any) => { + const source = { + foo: propValue, + }; + const name = 'foo'; + const replacement = vi.fn().mockImplementationOnce(cb => cb); + + fill(source, name, replacement); + + expect(source.foo).toEqual(propValue); + expect(replacement).not.toBeCalled(); + }); + test('can do anything inside replacement function', () => { const source = { foo: (): number => 42, From 859b33a8b3389ec82f723cf5580e6a0c675b8216 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 11 Mar 2025 16:28:18 +0100 Subject: [PATCH 3/3] explicitly type cast to unknown --- packages/core/src/utils-hoist/object.ts | 4 +++- packages/core/test/utils-hoist/object.test.ts | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/core/src/utils-hoist/object.ts b/packages/core/src/utils-hoist/object.ts index 3c2b04129e9e..2c9080682a9d 100644 --- a/packages/core/src/utils-hoist/object.ts +++ b/packages/core/src/utils-hoist/object.ts @@ -25,7 +25,9 @@ export function fill(source: { [key: string]: any }, name: string, replacementFa return; } - const original = source[name]; + // explicitly casting to unknown because we don't know the type of the method initially at all + const original = source[name] as unknown; + if (typeof original !== 'function') { return; } diff --git a/packages/core/test/utils-hoist/object.test.ts b/packages/core/test/utils-hoist/object.test.ts index f620becbf451..e1c32f163193 100644 --- a/packages/core/test/utils-hoist/object.test.ts +++ b/packages/core/test/utils-hoist/object.test.ts @@ -63,7 +63,7 @@ describe('fill()', () => { fill(source, name, replacement); - expect(source.foo).toEqual(propValue); + expect(source.foo).toBe(propValue); expect(replacement).not.toBeCalled(); });