From 4e210618847878b601928ea3d7187c8092542036 Mon Sep 17 00:00:00 2001 From: Thomas Lebeau Date: Tue, 3 Sep 2024 16:39:01 +0200 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20Allow=20both=20history=20and?= =?UTF-8?q?=20History=20prototype=20to=20be=20patched=20by=203rd=20party?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/core/test/emulate/mockLocation.ts | 23 +--- .../browser/locationChangeObservable.spec.ts | 104 ++++++++++++++---- .../src/browser/locationChangeObservable.ts | 18 ++- 3 files changed, 96 insertions(+), 49 deletions(-) diff --git a/packages/core/test/emulate/mockLocation.ts b/packages/core/test/emulate/mockLocation.ts index ae6a9befd1..55e0316123 100644 --- a/packages/core/test/emulate/mockLocation.ts +++ b/packages/core/test/emulate/mockLocation.ts @@ -1,25 +1,4 @@ -import { assign, buildUrl } from '../../src' - -export function mockLocation(initialUrl: string) { - const fakeLocation = buildLocation(initialUrl) - spyOn(History.prototype, 'pushState').and.callFake((_: any, __: string, pathname: string) => { - assign(fakeLocation, buildLocation(pathname, fakeLocation.href)) - }) - - function hashchangeCallBack() { - fakeLocation.hash = window.location.hash - fakeLocation.href = fakeLocation.href.replace(/#.*/, '') + window.location.hash - } - - window.addEventListener('hashchange', hashchangeCallBack) - return { - location: fakeLocation, - cleanup: () => { - window.removeEventListener('hashchange', hashchangeCallBack) - window.location.hash = '' - }, - } -} +import { buildUrl } from '../../src' export function buildLocation(url: string, base = location.href) { const urlObject = buildUrl(url, base) diff --git a/packages/rum-core/src/browser/locationChangeObservable.spec.ts b/packages/rum-core/src/browser/locationChangeObservable.spec.ts index 08ffbf5ef2..a49a220a58 100644 --- a/packages/rum-core/src/browser/locationChangeObservable.spec.ts +++ b/packages/rum-core/src/browser/locationChangeObservable.spec.ts @@ -1,31 +1,11 @@ -import type { Observable, Subscription } from '@datadog/browser-core' -import { mockLocation } from '@datadog/browser-core/test' +import { registerCleanupTask } from '@datadog/browser-core/test' import type { RumConfiguration } from '@datadog/browser-rum-core' -import type { LocationChange } from './locationChangeObservable' import { createLocationChangeObservable } from './locationChangeObservable' describe('locationChangeObservable', () => { - let observable: Observable - let observer: jasmine.Spy<(locationChange: LocationChange) => void> - let subscription: Subscription - let fakeLocation: Partial - let cleanupLocation: () => void - let configuration: RumConfiguration - - beforeEach(() => { - ;({ location: fakeLocation, cleanup: cleanupLocation } = mockLocation('/foo')) - configuration = {} as RumConfiguration - observable = createLocationChangeObservable(configuration, fakeLocation as Location) - observer = jasmine.createSpy('obs') - subscription = observable.subscribe(observer) - }) - - afterEach(() => { - subscription.unsubscribe() - cleanupLocation() - }) - it('should notify observers on history change', () => { + const observer = setup() + history.pushState({}, '', '/foo?bar=qux') const locationChanges = observer.calls.argsFor(0)[0] @@ -34,6 +14,8 @@ describe('locationChangeObservable', () => { }) it('should notify observers on hashchange', (done) => { + const observer = setup() + function hashChangeCallback() { const locationChanges = observer.calls.argsFor(0)[0] expect(locationChanges.oldLocation.href).toMatch(/\/foo$/) @@ -48,8 +30,84 @@ describe('locationChangeObservable', () => { }) it('should not notify if the url has not changed', () => { + const observer = setup() + history.pushState({}, '', '/foo') expect(observer).not.toHaveBeenCalled() }) + + it('allow frameworks to patch history.pushState', () => { + const wrapperSpy = setupHistoryInstancePushStateWrapper() + const observer = setup() + + history.pushState({}, '', '/foo?bar=qux') + + const locationChanges = observer.calls.argsFor(0)[0] + expect(locationChanges.oldLocation.href).toMatch(/\/foo$/) + expect(locationChanges.newLocation.href).toMatch(/\/foo\?bar=qux$/) + expect(wrapperSpy).toHaveBeenCalled() + }) + + it('allow frameworks to patch History.prototype.pushState', () => { + const wrapperSpy = setupHistoryPrototypePushStateWrapper() + const observer = setup() + + history.pushState({}, '', '/foo?bar=qux') + + const locationChanges = observer.calls.argsFor(0)[0] + expect(locationChanges.oldLocation.href).toMatch(/\/foo$/) + expect(locationChanges.newLocation.href).toMatch(/\/foo\?bar=qux$/) + expect(wrapperSpy).toHaveBeenCalled() + }) }) + +function setup() { + const originalPathname = location.pathname + + history.pushState({}, '', '/foo') + + const observable = createLocationChangeObservable({} as RumConfiguration, location) + const observer = jasmine.createSpy('obs') + const subscription = observable.subscribe(observer) + + registerCleanupTask(() => { + subscription.unsubscribe() + history.pushState({}, '', originalPathname) + }) + + return observer +} + +function setupHistoryInstancePushStateWrapper() { + const wrapperSpy = jasmine.createSpy('wrapperSpy') + const originalPushState = history.pushState.bind(history) + + history.pushState = (...args) => { + wrapperSpy(...args) + originalPushState(...args) + } + + registerCleanupTask(() => { + // @ts-expect-error reseting history instance to its original state + delete history.pushState + }) + + return wrapperSpy +} + +function setupHistoryPrototypePushStateWrapper() { + const wrapperSpy = jasmine.createSpy('wrapperSpy') + const originalPushState = History.prototype.pushState.bind(history) + + History.prototype.pushState = (...args) => { + wrapperSpy(...args) + originalPushState(...args) + } + + registerCleanupTask(() => { + History.prototype.pushState = originalPushState + }) + + return wrapperSpy +} diff --git a/packages/rum-core/src/browser/locationChangeObservable.ts b/packages/rum-core/src/browser/locationChangeObservable.ts index b2df6d09fb..8136822e64 100644 --- a/packages/rum-core/src/browser/locationChangeObservable.ts +++ b/packages/rum-core/src/browser/locationChangeObservable.ts @@ -33,11 +33,15 @@ export function createLocationChangeObservable(configuration: RumConfiguration, } function trackHistory(configuration: RumConfiguration, onHistoryChange: () => void) { - const { stop: stopInstrumentingPushState } = instrumentMethod(History.prototype, 'pushState', ({ onPostCall }) => { - onPostCall(onHistoryChange) - }) + const { stop: stopInstrumentingPushState } = instrumentMethod( + getHistoryInstrumentationTarget('pushState'), + 'pushState', + ({ onPostCall }) => { + onPostCall(onHistoryChange) + } + ) const { stop: stopInstrumentingReplaceState } = instrumentMethod( - History.prototype, + getHistoryInstrumentationTarget('replaceState'), 'replaceState', ({ onPostCall }) => { onPostCall(onHistoryChange) @@ -57,3 +61,9 @@ function trackHistory(configuration: RumConfiguration, onHistoryChange: () => vo function trackHash(configuration: RumConfiguration, onHashChange: () => void) { return addEventListener(configuration, window, DOM_EVENT.HASH_CHANGE, onHashChange) } + +function getHistoryInstrumentationTarget(methodName: 'pushState' | 'replaceState') { + // Ideally we should always instument the method on the prototype, however some frameworks (e.g [Next.js](https://github.com/vercel/next.js/blob/d3f5532065f3e3bb84fb54bd2dfd1a16d0f03a21/packages/next/src/client/components/app-router.tsx#L429)) + // are wrapping the instance method. In that case we should also wrap the instance method. + return Object.prototype.hasOwnProperty.call(history, methodName) ? history : History.prototype +} From 1040139db3be45bb5850ddff3c099bfdaa88ee70 Mon Sep 17 00:00:00 2001 From: Thomas Lebeau Date: Wed, 4 Sep 2024 11:56:47 +0200 Subject: [PATCH 2/2] rename mockLocation.ts to buildLocation.ts --- .../core/test/emulate/{mockLocation.ts => buildLocation.ts} | 0 packages/core/test/index.ts | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename packages/core/test/emulate/{mockLocation.ts => buildLocation.ts} (100%) diff --git a/packages/core/test/emulate/mockLocation.ts b/packages/core/test/emulate/buildLocation.ts similarity index 100% rename from packages/core/test/emulate/mockLocation.ts rename to packages/core/test/emulate/buildLocation.ts diff --git a/packages/core/test/index.ts b/packages/core/test/index.ts index 003646b827..889042cb2d 100644 --- a/packages/core/test/index.ts +++ b/packages/core/test/index.ts @@ -5,7 +5,7 @@ export * from './cookie' export * from './registerCleanupTask' export * from './interceptRequests' export * from './emulate/createNewEvent' -export * from './emulate/mockLocation' +export * from './emulate/buildLocation' export * from './emulate/mockClock' export * from './emulate/mockReportingObserver' export * from './emulate/mockZoneJs'