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

SDK: better support for SPA routing - STEP 2/2 : filter out html anchor tags changes #466

Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
1c11c6e
add hash tracking
mquentin Jul 22, 2020
41dec89
✅ add hash change tests
mquentin Jul 22, 2020
f5565c2
introduce renewViewOnChange
mquentin Jul 22, 2020
72ae621
add better management and comments of the areViewDifferent hash part
mquentin Jul 22, 2020
571e702
set async tests for hashchanges
mquentin Jul 23, 2020
fc6d677
reset window.location.hash of previous tests
mquentin Jul 23, 2020
97192a5
skip tesst using Promise for IE
mquentin Jul 23, 2020
9383bf1
replace jasmine promise with callback process
mquentin Jul 27, 2020
0e2d6b5
remove anchor check on this PR
mquentin Jul 27, 2020
19bc93f
add anchor nav filter
mquentin Jul 27, 2020
a86a41c
patch tslint error
mquentin Jul 27, 2020
52aafd5
✅ and implem reviews
mquentin Jul 28, 2020
29d6312
implement reviews regarding func naming and hash cleaning
mquentin Jul 28, 2020
e593a8c
Merge branch 'maxime.quentin/betterSupportForSPAroutingAddhashTrackin…
mquentin Jul 28, 2020
27b6d52
simplify mockGetElementById
mquentin Jul 28, 2020
5bd0103
Merge branch 'master' into maxime.quentin/betterSupportForSPAroutingF…
mquentin Jul 28, 2020
4cee0ad
Merge branch 'maxime.quentin/betterSupportForSPArouting' into maxime.…
mquentin Jul 28, 2020
07ba398
add e2e tests
mquentin Jul 30, 2020
f282a53
simply anchor filter out proccess
mquentin Jul 30, 2020
2063209
implement reviews
mquentin Jul 30, 2020
ed4f9be
unit test the hash change acknowledgement after an anchor nav
mquentin Jul 30, 2020
9336bce
fix typo
mquentin Jul 30, 2020
1f809c3
fix typo v2
mquentin Jul 30, 2020
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
11 changes: 10 additions & 1 deletion packages/rum/src/viewCollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 interfer with the next areDifferentViews call
mquentin marked this conversation as resolved.
Show resolved Hide resolved
currentLocation = { ...location }
mquentin marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -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 {
Expand Down
45 changes: 45 additions & 0 deletions packages/rum/test/viewCollection.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@ function mockHash(location: Partial<Location>) {
}
}

function mockGetElementById() {
return spyOn(document, 'getElementById').and.callFake((elementId: string) => {
return (elementId === ('testHashValue' as unknown)) as any
})
}

function spyOnViews() {
const handler = jasmine.createSpy()

Expand Down Expand Up @@ -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) => {
bcaudan marked this conversation as resolved.
Show resolved Hide resolved
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()
Copy link
Contributor

Choose a reason for hiding this comment

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

what about something more expressive than spyObj?

Copy link
Member Author

@mquentin mquentin Jul 30, 2020

Choose a reason for hiding this comment

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

as we just "clear" it, I did not look for a better name. Also the comment helps understanding the usage of spyObj.and.callThrough()

Copy link
Contributor

Choose a reason for hiding this comment

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

it could have been an opportunity to be more expressive

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)
Expand Down
29 changes: 28 additions & 1 deletion test/e2e/scenario/agents.scenario.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
waitServerRumEvents,
withBrowserLogs,
} from './helpers'
import { isRumResourceEvent, isRumUserActionEvent, isRumViewEvent } from './serverTypes'
import { isRumResourceEvent, isRumUserActionEvent, isRumViewEvent, ServerRumViewLoadingType } from './serverTypes'

beforeEach(startSpec)

Expand Down Expand Up @@ -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)
})
})
6 changes: 6 additions & 0 deletions test/e2e/scenario/serverTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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
Expand Down
3 changes: 3 additions & 0 deletions test/static/async-e2e-page.html
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
</head>
<body>
<button>click me</button>
<div id="test-anchor">
<a href="#test-anchor">anchor link</a>
</div>
<script type="text/javascript">
window.addEventListener('load', () => {
const intakeOrigin = `http://${window.location.hostname}:4000`
Expand Down
3 changes: 3 additions & 0 deletions test/static/bundle-e2e-page.html
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,8 @@
</head>
<body>
<button>click me</button>
<div id="test-anchor">
<a href="#test-anchor">anchor link</a>
</div>
</body>
</html>
3 changes: 3 additions & 0 deletions test/static/npm-e2e-page.html
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,8 @@
</head>
<body>
<button>click me</button>
<div id="test-anchor">
<a href="#test-anchor">anchor link</a>
</div>
</body>
</html>