Skip to content

fix(core): Ensure fill only patches functions #15632

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

Merged
merged 3 commits into from
Mar 11, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
13 changes: 10 additions & 3 deletions packages/browser-utils/src/instrument/history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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);
}
}
58 changes: 58 additions & 0 deletions packages/browser-utils/test/instrument/history.test.ts
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

prior to this PR, pushState would have become a Function, resulting in the wrapper function calling undefined.apply on the "original function".

});
});

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
});
});
});
2 changes: 1 addition & 1 deletion packages/core/src/utils-hoist/supports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member Author

Choose a reason for hiding this comment

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

Improved this check so that if history was undefined or null we'd also return false here

}

/**
Expand Down
32 changes: 32 additions & 0 deletions packages/core/test/utils-hoist/supports.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
Loading