From 57391e2140a85d99cc01fbedf1773cb89fa3aaef Mon Sep 17 00:00:00 2001 From: Maxime Quentin Date: Tue, 28 Jul 2020 14:38:39 +0200 Subject: [PATCH 1/3] SDK: better support for SPA routing - STEP 1/2 : add hash tracking (#463) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * add hash tracking * ✅ add hash change tests * introduce renewViewOnChange * add better management and comments of the areViewDifferent hash part * set async tests for hashchanges * reset window.location.hash of previous tests * skip tesst using Promise for IE * replace jasmine promise with callback process * remove anchor check on this PR * ✅ and implem reviews --- packages/rum/src/viewCollection.ts | 19 ++++-- packages/rum/test/viewCollection.spec.ts | 75 ++++++++++++++++++++++-- 2 files changed, 83 insertions(+), 11 deletions(-) diff --git a/packages/rum/src/viewCollection.ts b/packages/rum/src/viewCollection.ts index c3ebcaa826..ced8a5c556 100644 --- a/packages/rum/src/viewCollection.ts +++ b/packages/rum/src/viewCollection.ts @@ -41,15 +41,20 @@ export function startViewCollection(location: Location, lifeCycle: LifeCycle) { const startOrigin = 0 let currentView = newView(lifeCycle, currentLocation, ViewLoadingType.INITIAL_LOAD, startOrigin) - // Renew view on history changes - trackHistory(() => { + function renewViewOnChange() { if (areDifferentViews(currentLocation, location)) { currentLocation = { ...location } currentView.triggerUpdate() currentView.end() currentView = newView(lifeCycle, currentLocation, ViewLoadingType.ROUTE_CHANGE) } - }) + } + + // Renew view on history changes + trackHistory(renewViewOnChange) + + // Renew view on hash changes + trackHash(renewViewOnChange) // Renew view on session renewal lifeCycle.subscribe(LifeCycleEventType.SESSION_RENEWED, () => { @@ -165,8 +170,12 @@ function trackHistory(onHistoryChange: () => void) { window.addEventListener(DOM_EVENT.POP_STATE, monitor(onHistoryChange)) } -function areDifferentViews(previous: Location, current: Location) { - return previous.pathname !== current.pathname +function trackHash(onHashChange: () => void) { + window.addEventListener('hashchange', monitor(onHashChange)) +} + +function areDifferentViews(previous: Location, current: Location): boolean { + return previous.pathname !== current.pathname || previous.hash !== current.hash } interface Timings { diff --git a/packages/rum/test/viewCollection.spec.ts b/packages/rum/test/viewCollection.spec.ts index 5e9cb99dee..2122c82f60 100644 --- a/packages/rum/test/viewCollection.spec.ts +++ b/packages/rum/test/viewCollection.spec.ts @@ -1,4 +1,4 @@ -import { getHash, getPathName, getSearch } from '@datadog/browser-core' +import { getHash, getPathName, getSearch, noop } from '@datadog/browser-core' import { LifeCycleEventType } from '../src/lifeCycle' import { ViewContext } from '../src/parentContexts' @@ -66,10 +66,23 @@ function mockHistory(location: Partial) { const url = `http://localhost${pathname}` location.pathname = getPathName(url) location.search = getSearch(url) - location.hash = getHash(url) + location.hash = getHash(url) || '' }) } +function mockHash(location: Partial) { + function hashchangeCallBack() { + location.hash = window.location.hash + } + + window.addEventListener('hashchange', hashchangeCallBack) + + return () => { + window.removeEventListener('hashchange', hashchangeCallBack) + window.location.hash = '' + } +} + function spyOnViews() { const handler = jasmine.createSpy() @@ -88,10 +101,12 @@ describe('rum track url change', () => { let setupBuilder: TestSetupBuilder let initialViewId: string let createSpy: jasmine.Spy + let cleanMockHash: () => void beforeEach(() => { - const fakeLocation: Partial = { pathname: '/foo' } + const fakeLocation: Partial = { pathname: '/foo', hash: '' } mockHistory(fakeLocation) + cleanMockHash = mockHash(fakeLocation) setupBuilder = setup() .withFakeLocation(fakeLocation) .withViewCollection() @@ -106,6 +121,7 @@ describe('rum track url change', () => { afterEach(() => { setupBuilder.cleanup() + cleanMockHash() }) it('should create new view on path change', () => { @@ -119,21 +135,68 @@ describe('rum track url change', () => { expect(viewContext.id).not.toEqual(initialViewId) }) - it('should not create new view on search change', () => { + it('should create a new view on hash change from history', () => { const { lifeCycle } = setupBuilder.build() lifeCycle.subscribe(LifeCycleEventType.VIEW_CREATED, createSpy) - history.pushState({}, '', '/foo?bar=qux') + history.pushState({}, '', '/foo#bar') + + expect(createSpy).toHaveBeenCalled() + const viewContext = createSpy.calls.argsFor(0)[0] as ViewContext + expect(viewContext.id).not.toEqual(initialViewId) + }) + + it('should not create a new view on hash change from history when the hash has kept the same value', () => { + history.pushState({}, '', '/foo#bar') + + const { lifeCycle } = setupBuilder.build() + lifeCycle.subscribe(LifeCycleEventType.VIEW_CREATED, createSpy) + + history.pushState({}, '', '/foo#bar') expect(createSpy).not.toHaveBeenCalled() }) - it('should not create a new view on hash change', () => { + it('should create a new view on hash change', (done) => { const { lifeCycle } = setupBuilder.build() lifeCycle.subscribe(LifeCycleEventType.VIEW_CREATED, createSpy) + function hashchangeCallBack() { + expect(createSpy).toHaveBeenCalled() + const viewContext = createSpy.calls.argsFor(0)[0] as ViewContext + expect(viewContext.id).not.toEqual(initialViewId) + window.removeEventListener('hashchange', hashchangeCallBack) + done() + } + + window.addEventListener('hashchange', hashchangeCallBack) + + window.location.hash = '#bar' + }) + + it('should not create a new view when the hash has kept the same value', (done) => { history.pushState({}, '', '/foo#bar') + const { lifeCycle } = setupBuilder.build() + lifeCycle.subscribe(LifeCycleEventType.VIEW_CREATED, createSpy) + + function hashchangeCallBack() { + expect(createSpy).not.toHaveBeenCalled() + window.removeEventListener('hashchange', hashchangeCallBack) + done() + } + + window.addEventListener('hashchange', hashchangeCallBack) + + window.location.hash = '#bar' + }) + + it('should not create new view on search change', () => { + const { lifeCycle } = setupBuilder.build() + lifeCycle.subscribe(LifeCycleEventType.VIEW_CREATED, createSpy) + + history.pushState({}, '', '/foo?bar=qux') + expect(createSpy).not.toHaveBeenCalled() }) }) From 0ca4e32f27c228ade675c6a610382532dc5af9b6 Mon Sep 17 00:00:00 2001 From: Maxime Quentin Date: Thu, 30 Jul 2020 18:08:09 +0200 Subject: [PATCH 2/3] SDK: better support for SPA routing - STEP 2/2 : filter out html anchor tags changes (#466) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * add hash tracking * ✅ add hash change tests * introduce renewViewOnChange * add better management and comments of the areViewDifferent hash part * set async tests for hashchanges * reset window.location.hash of previous tests * skip tesst using Promise for IE * replace jasmine promise with callback process * remove anchor check on this PR * add anchor nav filter * patch tslint error * ✅ and implem reviews * implement reviews regarding func naming and hash cleaning * simplify mockGetElementById * add e2e tests * simply anchor filter out proccess * implement reviews * unit test the hash change acknowledgement after an anchor nav * fix typo * fix typo v2 --- packages/rum/src/viewCollection.ts | 11 +++++- packages/rum/test/viewCollection.spec.ts | 45 ++++++++++++++++++++++++ test/e2e/scenario/agents.scenario.ts | 29 ++++++++++++++- test/e2e/scenario/serverTypes.ts | 6 ++++ test/static/async-e2e-page.html | 3 ++ test/static/bundle-e2e-page.html | 3 ++ test/static/npm-e2e-page.html | 3 ++ 7 files changed, 98 insertions(+), 2 deletions(-) diff --git a/packages/rum/src/viewCollection.ts b/packages/rum/src/viewCollection.ts index ced8a5c556..12eb76bc3b 100644 --- a/packages/rum/src/viewCollection.ts +++ b/packages/rum/src/viewCollection.ts @@ -47,6 +47,10 @@ export function startViewCollection(location: Location, lifeCycle: LifeCycle) { currentView.triggerUpdate() currentView.end() currentView = newView(lifeCycle, currentLocation, ViewLoadingType.ROUTE_CHANGE) + } else { + // Anchor navigations would modify the location without generating a new view. + // These changes need to be acknowledged so they don't interfere with the next areDifferentViews call + currentLocation = { ...location } } } @@ -174,8 +178,13 @@ function trackHash(onHashChange: () => void) { window.addEventListener('hashchange', monitor(onHashChange)) } +function isHashAnAnchor(hash: string) { + const correspondingId = hash.substr(1) + return !!document.getElementById(correspondingId) +} + function areDifferentViews(previous: Location, current: Location): boolean { - return previous.pathname !== current.pathname || previous.hash !== current.hash + return previous.pathname !== current.pathname || (!isHashAnAnchor(current.hash) && previous.hash !== current.hash) } interface Timings { diff --git a/packages/rum/test/viewCollection.spec.ts b/packages/rum/test/viewCollection.spec.ts index 2122c82f60..a1caae353b 100644 --- a/packages/rum/test/viewCollection.spec.ts +++ b/packages/rum/test/viewCollection.spec.ts @@ -83,6 +83,12 @@ function mockHash(location: Partial) { } } +function mockGetElementById() { + return spyOn(document, 'getElementById').and.callFake((elementId: string) => { + return (elementId === ('testHashValue' as unknown)) as any + }) +} + function spyOnViews() { const handler = jasmine.createSpy() @@ -191,6 +197,45 @@ describe('rum track url change', () => { window.location.hash = '#bar' }) + it('should not create a new view when it is an Anchor navigation', (done) => { + const { lifeCycle } = setupBuilder.build() + mockGetElementById() + lifeCycle.subscribe(LifeCycleEventType.VIEW_CREATED, createSpy) + + function hashchangeCallBack() { + expect(createSpy).not.toHaveBeenCalled() + window.removeEventListener('hashchange', hashchangeCallBack) + done() + } + + window.addEventListener('hashchange', hashchangeCallBack) + + window.location.hash = '#testHashValue' + }) + + it('should acknowledge the view location hash change after an Anchor navigation', (done) => { + const { lifeCycle } = setupBuilder.build() + const spyObj = mockGetElementById() + lifeCycle.subscribe(LifeCycleEventType.VIEW_CREATED, createSpy) + + function hashchangeCallBack() { + expect(createSpy).not.toHaveBeenCalled() + window.removeEventListener('hashchange', hashchangeCallBack) + + // clear mockGetElementById that fake Anchor nav + spyObj.and.callThrough() + + // This is not an Anchor nav anymore but the hash and pathname have not been updated + history.pushState({}, '', '/foo#testHashValue') + expect(createSpy).not.toHaveBeenCalled() + done() + } + + window.addEventListener('hashchange', hashchangeCallBack) + + window.location.hash = '#testHashValue' + }) + it('should not create new view on search change', () => { const { lifeCycle } = setupBuilder.build() lifeCycle.subscribe(LifeCycleEventType.VIEW_CREATED, createSpy) diff --git a/test/e2e/scenario/agents.scenario.ts b/test/e2e/scenario/agents.scenario.ts index 595a7d6853..d878be7783 100644 --- a/test/e2e/scenario/agents.scenario.ts +++ b/test/e2e/scenario/agents.scenario.ts @@ -16,7 +16,7 @@ import { waitServerRumEvents, withBrowserLogs, } from './helpers' -import { isRumResourceEvent, isRumUserActionEvent, isRumViewEvent } from './serverTypes' +import { isRumResourceEvent, isRumUserActionEvent, isRumViewEvent, ServerRumViewLoadingType } from './serverTypes' beforeEach(startSpec) @@ -270,3 +270,30 @@ describe('user action collection', () => { expect(resourceEvents[0].user_action!.id).toBe(userActionEvents[0].user_action.id!) }) }) + +describe('anchor navigation', () => { + it('should not create a new view when it is an Anchor navigation', async () => { + await $('#test-anchor').click() + + await flushEvents() + const rumEvents = await waitServerRumEvents() + const viewEvents = rumEvents.filter(isRumViewEvent) + + expect(viewEvents.length).toBe(1) + expect(viewEvents[0].view.loading_type).toBe(ServerRumViewLoadingType.INITIAL_LOAD) + }) + + it('should create a new view on hash change', async () => { + await browserExecute(() => { + window.location.hash = '#bar' + }) + + await flushEvents() + const rumEvents = await waitServerRumEvents() + const viewEvents = rumEvents.filter(isRumViewEvent) + + expect(viewEvents.length).toBe(2) + expect(viewEvents[0].view.loading_type).toBe(ServerRumViewLoadingType.INITIAL_LOAD) + expect(viewEvents[1].view.loading_type).toBe(ServerRumViewLoadingType.ROUTE_CHANGE) + }) +}) diff --git a/test/e2e/scenario/serverTypes.ts b/test/e2e/scenario/serverTypes.ts index 0339cbb4d4..c988fa78e2 100644 --- a/test/e2e/scenario/serverTypes.ts +++ b/test/e2e/scenario/serverTypes.ts @@ -80,6 +80,11 @@ export function isRumUserActionEvent(event: ServerRumEvent): event is ServerRumU return event.evt.category === 'user_action' } +export enum ServerRumViewLoadingType { + INITIAL_LOAD = 'initial_load', + ROUTE_CHANGE = 'route_change', +} + export interface ServerRumViewEvent extends ServerRumEvent { evt: { category: 'view' @@ -90,6 +95,7 @@ export interface ServerRumViewEvent extends ServerRumEvent { session_id: string view: { id: string + loading_type: ServerRumViewLoadingType measures: { dom_complete: number dom_content_loaded: number diff --git a/test/static/async-e2e-page.html b/test/static/async-e2e-page.html index 0bc874d027..0fd6dec29a 100644 --- a/test/static/async-e2e-page.html +++ b/test/static/async-e2e-page.html @@ -9,6 +9,9 @@ +