From 21a007d2550979981308f8c94e912c18bd5ab078 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 13:45:46 -0500 Subject: [PATCH 01/15] add differences --- .../src/web/web-signals-types.ts | 4 +++ .../core/signal-generators/dom-gen/dom-gen.ts | 27 +++++++++++++++++-- .../src/lib/detect-url-change/index.ts | 25 ++++++++++++++--- 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/packages/signals/signals-runtime/src/web/web-signals-types.ts b/packages/signals/signals-runtime/src/web/web-signals-types.ts index d5a17b569..ee3d8fd80 100644 --- a/packages/signals/signals-runtime/src/web/web-signals-types.ts +++ b/packages/signals/signals-runtime/src/web/web-signals-types.ts @@ -106,11 +106,15 @@ interface BaseNavigationData { action: ActionType url: string hash: string + search: string + path: string } +export type ChangedProperties = 'path' | 'search' | 'hash' export interface URLChangeNavigationData extends BaseNavigationData<'urlChange'> { prevUrl: string + changedProperties: ChangedProperties[] } export interface PageChangeNavigationData diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts index b45ca1ec3..f7718beb2 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts @@ -1,3 +1,4 @@ +import { ChangedProperties } from '@segment/analytics-signals-runtime' import { URLChangeObservable } from '../../../lib/detect-url-change' import { createInteractionSignal, @@ -79,6 +80,27 @@ export const shouldIgnoreElement = (el: HTMLElement): boolean => { return false } +function getObjectDifferences(obj1: T, obj2: T): string[] { + const keys = new Set([...Object.keys(obj1), ...Object.keys(obj2)]) + return Array.from(keys).filter( + (key) => obj1[key as keyof T] !== obj2[key as keyof T] + ) +} + +function getURLDifferences(current: URL, previous: URL): ChangedProperties[] { + const difference = getObjectDifferences(current, previous) + const changed: ChangedProperties[] = [] + for (const k of difference) { + if (k === 'pathname') { + changed.push('path') + } + if (['search', 'hash'].includes(k)) { + changed.push(k as ChangedProperties) + } + } + return changed +} + export class OnNavigationEventGenerator implements SignalGenerator { id = 'navigation' @@ -93,11 +115,12 @@ export class OnNavigationEventGenerator implements SignalGenerator { // emit a navigation signal whenever the URL has changed const urlChange = new URLChangeObservable() - urlChange.subscribe((prevUrl) => + urlChange.subscribe(({ previous, current }) => emitter.emit( createNavigationSignal({ action: 'urlChange', - prevUrl, + prevUrl: previous.href, + changedProperties: getURLDifferences(current, previous), ...this.createCommonFields(), }) ) diff --git a/packages/signals/signals/src/lib/detect-url-change/index.ts b/packages/signals/signals/src/lib/detect-url-change/index.ts index 72a2d50c3..c2cf4d762 100644 --- a/packages/signals/signals/src/lib/detect-url-change/index.ts +++ b/packages/signals/signals/src/lib/detect-url-change/index.ts @@ -1,11 +1,23 @@ import { Emitter } from '@segment/analytics-generic-utils' +const safeParseURL = (url: string): URL | null => { + try { + return new URL(url) + } catch (e) { + return null + } +} + +type ChangeData = { + current: URL + previous: URL +} // This seems hacky, but if using react router (or other SPAs), popstate will not always fire on navigation // Otherwise, we could use popstate / hashchange events export class URLChangeObservable { - private emitter = new Emitter() + private emitter = new Emitter<{ change: [ChangeData] }>() private pollInterval = 500 - urlChanged?: (url: string) => void + private urlChanged?: (data: ChangeData) => void constructor() { this.pollURLChange() } @@ -15,13 +27,18 @@ export class URLChangeObservable { setInterval(() => { const currentUrl = window.location.href if (currentUrl != prevUrl) { - this.emitter.emit('change', prevUrl) + const current = safeParseURL(currentUrl)! + const prev = safeParseURL(prevUrl)! + this.emitter.emit('change', { + current, + previous: prev, + }) prevUrl = currentUrl } }, this.pollInterval) } - subscribe(urlChanged: (newURL: string) => void) { + subscribe(urlChanged: (data: ChangeData) => void) { this.urlChanged = urlChanged this.emitter.on('change', this.urlChanged) } From 1cfc24be353dcef3650cc56fc926ef8a67e6fb33 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 13:48:17 -0500 Subject: [PATCH 02/15] wip --- .../signals/src/lib/detect-url-change/index.ts | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/packages/signals/signals/src/lib/detect-url-change/index.ts b/packages/signals/signals/src/lib/detect-url-change/index.ts index c2cf4d762..65aee3cef 100644 --- a/packages/signals/signals/src/lib/detect-url-change/index.ts +++ b/packages/signals/signals/src/lib/detect-url-change/index.ts @@ -1,13 +1,5 @@ import { Emitter } from '@segment/analytics-generic-utils' -const safeParseURL = (url: string): URL | null => { - try { - return new URL(url) - } catch (e) { - return null - } -} - type ChangeData = { current: URL previous: URL @@ -27,8 +19,8 @@ export class URLChangeObservable { setInterval(() => { const currentUrl = window.location.href if (currentUrl != prevUrl) { - const current = safeParseURL(currentUrl)! - const prev = safeParseURL(prevUrl)! + const current = new URL(currentUrl) + const prev = new URL(prevUrl) this.emitter.emit('change', { current, previous: prev, From 48e5490b853a2ab42e9f165dcbb9215bef9ec8ed Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 14:02:33 -0500 Subject: [PATCH 03/15] wip --- .../core/signal-generators/dom-gen/dom-gen.ts | 29 +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts index f7718beb2..816909baa 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts @@ -80,15 +80,32 @@ export const shouldIgnoreElement = (el: HTMLElement): boolean => { return false } -function getObjectDifferences(obj1: T, obj2: T): string[] { - const keys = new Set([...Object.keys(obj1), ...Object.keys(obj2)]) - return Array.from(keys).filter( - (key) => obj1[key as keyof T] !== obj2[key as keyof T] - ) +export function getDifferentProperties(url1: URL, url2: URL): string[] { + const differentProperties: string[] = [] + const propertiesToCompare = [ + 'href', + 'protocol', + 'host', + 'hostname', + 'port', + 'pathname', + 'search', + 'hash', + 'username', + 'password', + ] + + for (const property of propertiesToCompare) { + if (url1[property as keyof URL] !== url2[property as keyof URL]) { + differentProperties.push(property) + } + } + + return differentProperties } function getURLDifferences(current: URL, previous: URL): ChangedProperties[] { - const difference = getObjectDifferences(current, previous) + const difference = getDifferentProperties(current, previous) const changed: ChangedProperties[] = [] for (const k of difference) { if (k === 'pathname') { From 7bc932b899dd386d7c3458fe07a986fd85af4f03 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 14:05:39 -0500 Subject: [PATCH 04/15] wip --- .../core/signal-generators/dom-gen/dom-gen.ts | 29 +++++++------------ 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts index 816909baa..44b5214b5 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts @@ -80,9 +80,9 @@ export const shouldIgnoreElement = (el: HTMLElement): boolean => { return false } -export function getDifferentProperties(url1: URL, url2: URL): string[] { - const differentProperties: string[] = [] - const propertiesToCompare = [ +function getURLDifferences(url1: URL, url2: URL): ChangedProperties[] { + const changed: ChangedProperties[] = [] + const propertiesToCompare: (keyof URL)[] = [ 'href', 'protocol', 'host', @@ -96,25 +96,16 @@ export function getDifferentProperties(url1: URL, url2: URL): string[] { ] for (const property of propertiesToCompare) { - if (url1[property as keyof URL] !== url2[property as keyof URL]) { - differentProperties.push(property) + if (url1[property] !== url2[property]) { + if (property === 'pathname') { + changed.push('path') + } + if (['search', 'hash'].includes(property)) { + changed.push(property as ChangedProperties) + } } } - return differentProperties -} - -function getURLDifferences(current: URL, previous: URL): ChangedProperties[] { - const difference = getDifferentProperties(current, previous) - const changed: ChangedProperties[] = [] - for (const k of difference) { - if (k === 'pathname') { - changed.push('path') - } - if (['search', 'hash'].includes(k)) { - changed.push(k as ChangedProperties) - } - } return changed } From 1ea69f7ab7a3cde8207ce65fa7bb382206384885 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 15:35:53 -0500 Subject: [PATCH 05/15] move navigation gen to seperate file --- .../core/signal-generators/dom-gen/dom-gen.ts | 71 ----------------- .../core/signal-generators/dom-gen/index.ts | 7 +- .../dom-gen/navigation-gen.ts | 76 +++++++++++++++++++ 3 files changed, 78 insertions(+), 76 deletions(-) create mode 100644 packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts index 44b5214b5..38f0638ae 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts @@ -79,74 +79,3 @@ export const shouldIgnoreElement = (el: HTMLElement): boolean => { } return false } - -function getURLDifferences(url1: URL, url2: URL): ChangedProperties[] { - const changed: ChangedProperties[] = [] - const propertiesToCompare: (keyof URL)[] = [ - 'href', - 'protocol', - 'host', - 'hostname', - 'port', - 'pathname', - 'search', - 'hash', - 'username', - 'password', - ] - - for (const property of propertiesToCompare) { - if (url1[property] !== url2[property]) { - if (property === 'pathname') { - changed.push('path') - } - if (['search', 'hash'].includes(property)) { - changed.push(property as ChangedProperties) - } - } - } - - return changed -} - -export class OnNavigationEventGenerator implements SignalGenerator { - id = 'navigation' - - register(emitter: SignalEmitter): () => void { - // emit navigation signal on page load - emitter.emit( - createNavigationSignal({ - action: 'pageLoad', - ...this.createCommonFields(), - }) - ) - - // emit a navigation signal whenever the URL has changed - const urlChange = new URLChangeObservable() - urlChange.subscribe(({ previous, current }) => - emitter.emit( - createNavigationSignal({ - action: 'urlChange', - prevUrl: previous.href, - changedProperties: getURLDifferences(current, previous), - ...this.createCommonFields(), - }) - ) - ) - - return () => { - urlChange.unsubscribe() - } - } - - private createCommonFields() { - return { - // these fields are named after those from the page call, rather than a DOM api. - url: location.href, - path: location.pathname, - hash: location.hash, - search: location.search, - title: document.title, - } - } -} diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/index.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/index.ts index 1c2675154..7107d575b 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/index.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/index.ts @@ -1,14 +1,11 @@ -import { - ClickSignalsGenerator, - FormSubmitGenerator, - OnNavigationEventGenerator, -} from './dom-gen' +import { ClickSignalsGenerator, FormSubmitGenerator } from './dom-gen' import { MutationChangeGenerator, OnChangeGenerator, ContentEditableChangeGenerator, } from './change-gen' import { SignalGeneratorClass } from '../types' +import { OnNavigationEventGenerator } from './navigation-gen' export const domGenerators: SignalGeneratorClass[] = [ MutationChangeGenerator, diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts new file mode 100644 index 000000000..ae046ec50 --- /dev/null +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts @@ -0,0 +1,76 @@ +import { ChangedProperties } from '@segment/analytics-signals-runtime' +import { URLChangeObservable } from '../../../lib/detect-url-change' +import { createNavigationSignal } from '../../../types/factories' +import { SignalEmitter } from '../../emitter' +import { SignalGenerator } from '../types' + +function getURLDifferences(url1: URL, url2: URL): ChangedProperties[] { + const changed: ChangedProperties[] = [] + const propertiesToCompare: (keyof URL)[] = [ + 'href', + 'protocol', + 'host', + 'hostname', + 'port', + 'pathname', + 'search', + 'hash', + 'username', + 'password', + ] + + for (const property of propertiesToCompare) { + if (url1[property] !== url2[property]) { + if (property === 'pathname') { + changed.push('path') + } + if (['search', 'hash'].includes(property)) { + changed.push(property as ChangedProperties) + } + } + } + + return changed +} + +export class OnNavigationEventGenerator implements SignalGenerator { + id = 'navigation' + + register(emitter: SignalEmitter): () => void { + // emit navigation signal on page load + emitter.emit( + createNavigationSignal({ + action: 'pageLoad', + ...this.createCommonFields(), + }) + ) + + // emit a navigation signal whenever the URL has changed + const urlChange = new URLChangeObservable() + urlChange.subscribe(({ previous, current }) => + emitter.emit( + createNavigationSignal({ + action: 'urlChange', + prevUrl: previous.href, + changedProperties: getURLDifferences(current, previous), + ...this.createCommonFields(), + }) + ) + ) + + return () => { + urlChange.unsubscribe() + } + } + + private createCommonFields() { + return { + // these fields are named after those from the page call, rather than a DOM api. + url: location.href, + path: location.pathname, + hash: location.hash, + search: location.search, + title: document.title, + } + } +} From 34770044720c7c1773b2deaaa7ed65f80040ea38 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 16:14:26 -0500 Subject: [PATCH 06/15] add url change --- .../dom-gen/__tests__/navigation-gen.test.ts | 133 ++++++++++++++++++ .../core/signal-generators/dom-gen/dom-gen.ts | 7 +- .../dom-gen/navigation-gen.ts | 13 +- .../src/lib/detect-url-change/index.ts | 9 +- 4 files changed, 151 insertions(+), 11 deletions(-) create mode 100644 packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts new file mode 100644 index 000000000..f3f7441f3 --- /dev/null +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts @@ -0,0 +1,133 @@ +import { jest } from '@jest/globals' +import { URLChangeNavigationData } from '@segment/analytics-signals-runtime' +import { URLChangeObservableSettings } from '../../../../lib/detect-url-change' +import { setLocation } from '../../../../test-helpers/set-location' +import { SignalEmitter } from '../../../emitter' +import { OnNavigationEventGenerator } from '../navigation-gen' + +const originalLocation = window.location + +describe('OnNavigationEventGenerator', () => { + let emitter: SignalEmitter + let emitSpy: jest.SpiedFunction + + beforeEach(() => { + setLocation(originalLocation) + jest.useFakeTimers() + emitter = new SignalEmitter() + emitSpy = jest.spyOn(emitter, 'emit') + }) + + afterEach(() => { + jest.clearAllMocks() + }) + + it('should emit an event with action "pageLoad" on initialization', () => { + const settings: URLChangeObservableSettings = { pollInterval: 100 } + const generator = new OnNavigationEventGenerator(settings) + + generator.register(emitter) + expect(emitSpy).toHaveBeenCalledTimes(1) + expect(emitSpy.mock.lastCall).toMatchInlineSnapshot(` + [ + { + "anonymousId": "", + "data": { + "action": "pageLoad", + "hash": "", + "page": { + "hash": "", + "hostname": "localhost", + "path": "/", + "referrer": "", + "search": "", + "title": "", + "url": "http://localhost/", + }, + "path": "/", + "search": "", + "title": "", + "url": "http://localhost/", + }, + "timestamp": , + "type": "navigation", + }, + ] + `) + }) + + it('should emit an event with "action: urlChange" when the URL changes', () => { + const settings: URLChangeObservableSettings = { pollInterval: 100 } + const generator = new OnNavigationEventGenerator(settings) + + generator.register(emitter) + + // Simulate a URL change + const newUrl = new URL(location.href) + newUrl.pathname = '/new-path' + newUrl.search = '?query=123' + newUrl.hash = '#hello' + setLocation({ + href: newUrl.href, + pathname: newUrl.pathname, + search: newUrl.search, + hash: newUrl.hash, + }) + + // Advance the timers to trigger the polling + jest.advanceTimersByTime(settings.pollInterval! + 100) + + expect(emitSpy).toHaveBeenCalledTimes(2) + + expect(emitSpy.mock.lastCall).toMatchInlineSnapshot(` + [ + { + "anonymousId": "", + "data": { + "action": "urlChange", + "changedProperties": [ + "path", + "search", + "hash", + ], + "hash": "#hello", + "page": { + "hash": "#hello", + "hostname": "localhost", + "path": "/new-path", + "referrer": "", + "search": "?query=123", + "title": "", + "url": "http://localhost/new-path?query=123#hello", + }, + "path": "/new-path", + "prevUrl": "http://localhost/", + "search": "?query=123", + "title": "", + "url": "http://localhost/new-path?query=123#hello", + }, + "timestamp": , + "type": "navigation", + }, + ] + `) + }) + + it('should only list the property that actually changed in changedProperties, and no more/less', () => { + const settings: URLChangeObservableSettings = { pollInterval: 100 } + const generator = new OnNavigationEventGenerator(settings) + + generator.register(emitter) + + const newUrl = new URL(location.href) + newUrl.hash = '#hello' + setLocation({ + href: newUrl.href, + hash: newUrl.hash, + }) + + jest.advanceTimersByTime(settings.pollInterval! + 1000) + const lastCall = emitSpy.mock.lastCall![0].data as URLChangeNavigationData + expect(lastCall.changedProperties).toEqual(['hash']) + }) +}) diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts index 38f0638ae..5f20c2594 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/dom-gen.ts @@ -1,9 +1,4 @@ -import { ChangedProperties } from '@segment/analytics-signals-runtime' -import { URLChangeObservable } from '../../../lib/detect-url-change' -import { - createInteractionSignal, - createNavigationSignal, -} from '../../../types/factories' +import { createInteractionSignal } from '../../../types/factories' import { SignalEmitter } from '../../emitter' import { SignalGenerator } from '../types' import { parseElement } from './element-parser' diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts index ae046ec50..ca996b08e 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts @@ -1,5 +1,8 @@ import { ChangedProperties } from '@segment/analytics-signals-runtime' -import { URLChangeObservable } from '../../../lib/detect-url-change' +import { + URLChangeObservable, + URLChangeObservableSettings, +} from '../../../lib/detect-url-change' import { createNavigationSignal } from '../../../types/factories' import { SignalEmitter } from '../../emitter' import { SignalGenerator } from '../types' @@ -36,6 +39,11 @@ function getURLDifferences(url1: URL, url2: URL): ChangedProperties[] { export class OnNavigationEventGenerator implements SignalGenerator { id = 'navigation' + urlChange: URLChangeObservable + constructor(settings: URLChangeObservableSettings) { + this.urlChange = new URLChangeObservable(settings) + } + register(emitter: SignalEmitter): () => void { // emit navigation signal on page load emitter.emit( @@ -46,8 +54,7 @@ export class OnNavigationEventGenerator implements SignalGenerator { ) // emit a navigation signal whenever the URL has changed - const urlChange = new URLChangeObservable() - urlChange.subscribe(({ previous, current }) => + this.urlChange.subscribe(({ previous, current }) => emitter.emit( createNavigationSignal({ action: 'urlChange', diff --git a/packages/signals/signals/src/lib/detect-url-change/index.ts b/packages/signals/signals/src/lib/detect-url-change/index.ts index 65aee3cef..e5b74aa8d 100644 --- a/packages/signals/signals/src/lib/detect-url-change/index.ts +++ b/packages/signals/signals/src/lib/detect-url-change/index.ts @@ -4,13 +4,18 @@ type ChangeData = { current: URL previous: URL } +export interface URLChangeObservableSettings { + pollInterval?: number +} + // This seems hacky, but if using react router (or other SPAs), popstate will not always fire on navigation // Otherwise, we could use popstate / hashchange events export class URLChangeObservable { private emitter = new Emitter<{ change: [ChangeData] }>() - private pollInterval = 500 + private pollInterval: number private urlChanged?: (data: ChangeData) => void - constructor() { + constructor(settings: URLChangeObservableSettings = {}) { + this.pollInterval = settings.pollInterval ?? 500 this.pollURLChange() } From 96f17bc339f26ccb44b4258c550fc88b9912ecdf Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 16:15:33 -0500 Subject: [PATCH 07/15] add changed properties to navigation signals --- .changeset/polite-olives-thank.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/polite-olives-thank.md diff --git a/.changeset/polite-olives-thank.md b/.changeset/polite-olives-thank.md new file mode 100644 index 000000000..b979700d1 --- /dev/null +++ b/.changeset/polite-olives-thank.md @@ -0,0 +1,6 @@ +--- +'@segment/analytics-signals': patch +'@segment/analytics-signals-runtime': patch +--- + +Add a changedProperties array to navigation signals From 904433f7a21407e1bed4b9dd7fa14213ad2dbcda Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 16:19:14 -0500 Subject: [PATCH 08/15] update mock signal types --- .../src/test-helpers/mocks/mock-signal-types-web.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/signals/signals-runtime/src/test-helpers/mocks/mock-signal-types-web.ts b/packages/signals/signals-runtime/src/test-helpers/mocks/mock-signal-types-web.ts index cac623b45..4e4fe6843 100644 --- a/packages/signals/signals-runtime/src/test-helpers/mocks/mock-signal-types-web.ts +++ b/packages/signals/signals-runtime/src/test-helpers/mocks/mock-signal-types-web.ts @@ -43,8 +43,11 @@ export const mockInteractionSignal: InteractionSignal = { export const mockNavigationSignal: NavigationSignal = { type: 'navigation', data: { - page: mockPageData, action: 'urlChange', + changedProperties: ['path', 'search', 'hash'], + page: mockPageData, + path: '/', + search: '', url: 'https://example.com', hash: '#section1', prevUrl: 'https://example.com/home', From cef24a1977af8d0228771393a842fda5c8be421b Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 16:19:33 -0500 Subject: [PATCH 09/15] wip --- .../src/test-helpers/mocks/mock-signal-types-web.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/signals/signals-runtime/src/test-helpers/mocks/mock-signal-types-web.ts b/packages/signals/signals-runtime/src/test-helpers/mocks/mock-signal-types-web.ts index 4e4fe6843..e1f5b10d0 100644 --- a/packages/signals/signals-runtime/src/test-helpers/mocks/mock-signal-types-web.ts +++ b/packages/signals/signals-runtime/src/test-helpers/mocks/mock-signal-types-web.ts @@ -44,7 +44,7 @@ export const mockNavigationSignal: NavigationSignal = { type: 'navigation', data: { action: 'urlChange', - changedProperties: ['path', 'search', 'hash'], + changedProperties: ['path'], page: mockPageData, path: '/', search: '', From 0054558f71888f0067ca1c23cc60a7f9aec7a05c Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 16:20:49 -0500 Subject: [PATCH 10/15] wip --- .../src/core/signal-generators/dom-gen/navigation-gen.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts index ca996b08e..174e4b003 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts @@ -40,8 +40,8 @@ export class OnNavigationEventGenerator implements SignalGenerator { id = 'navigation' urlChange: URLChangeObservable - constructor(settings: URLChangeObservableSettings) { - this.urlChange = new URLChangeObservable(settings) + constructor() { + this.urlChange = new URLChangeObservable() } register(emitter: SignalEmitter): () => void { From be6370a19f88fffdf9af65427f672724e9001b16 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 16:21:14 -0500 Subject: [PATCH 11/15] wip --- .../src/core/signal-generators/dom-gen/navigation-gen.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts index 174e4b003..ccf8e8795 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts @@ -1,8 +1,5 @@ import { ChangedProperties } from '@segment/analytics-signals-runtime' -import { - URLChangeObservable, - URLChangeObservableSettings, -} from '../../../lib/detect-url-change' +import { URLChangeObservable } from '../../../lib/detect-url-change' import { createNavigationSignal } from '../../../types/factories' import { SignalEmitter } from '../../emitter' import { SignalGenerator } from '../types' @@ -66,7 +63,7 @@ export class OnNavigationEventGenerator implements SignalGenerator { ) return () => { - urlChange.unsubscribe() + this.urlChange.unsubscribe() } } From 3819d02dbc48789f088126f2a858b65a91e5b721 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 16:23:12 -0500 Subject: [PATCH 12/15] wip --- .../dom-gen/__tests__/navigation-gen.test.ts | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts index f3f7441f3..f2637e8d4 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts @@ -1,6 +1,5 @@ import { jest } from '@jest/globals' import { URLChangeNavigationData } from '@segment/analytics-signals-runtime' -import { URLChangeObservableSettings } from '../../../../lib/detect-url-change' import { setLocation } from '../../../../test-helpers/set-location' import { SignalEmitter } from '../../../emitter' import { OnNavigationEventGenerator } from '../navigation-gen' @@ -23,9 +22,7 @@ describe('OnNavigationEventGenerator', () => { }) it('should emit an event with action "pageLoad" on initialization', () => { - const settings: URLChangeObservableSettings = { pollInterval: 100 } - const generator = new OnNavigationEventGenerator(settings) - + const generator = new OnNavigationEventGenerator() generator.register(emitter) expect(emitSpy).toHaveBeenCalledTimes(1) expect(emitSpy.mock.lastCall).toMatchInlineSnapshot(` @@ -57,8 +54,7 @@ describe('OnNavigationEventGenerator', () => { }) it('should emit an event with "action: urlChange" when the URL changes', () => { - const settings: URLChangeObservableSettings = { pollInterval: 100 } - const generator = new OnNavigationEventGenerator(settings) + const generator = new OnNavigationEventGenerator() generator.register(emitter) @@ -75,7 +71,7 @@ describe('OnNavigationEventGenerator', () => { }) // Advance the timers to trigger the polling - jest.advanceTimersByTime(settings.pollInterval! + 100) + jest.advanceTimersByTime(1000) expect(emitSpy).toHaveBeenCalledTimes(2) @@ -114,8 +110,7 @@ describe('OnNavigationEventGenerator', () => { }) it('should only list the property that actually changed in changedProperties, and no more/less', () => { - const settings: URLChangeObservableSettings = { pollInterval: 100 } - const generator = new OnNavigationEventGenerator(settings) + const generator = new OnNavigationEventGenerator() generator.register(emitter) @@ -126,7 +121,7 @@ describe('OnNavigationEventGenerator', () => { hash: newUrl.hash, }) - jest.advanceTimersByTime(settings.pollInterval! + 1000) + jest.advanceTimersByTime(1000) const lastCall = emitSpy.mock.lastCall![0].data as URLChangeNavigationData expect(lastCall.changedProperties).toEqual(['hash']) }) From df227c51123974e27d56dbbb07f00f00544f3721 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 16:29:11 -0500 Subject: [PATCH 13/15] wip --- .../dom-gen/__tests__/navigation-gen.test.ts | 2 +- .../signal-generators/dom-gen/navigation-gen.ts | 16 ++-------------- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts index f2637e8d4..b5bc7c371 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts @@ -6,7 +6,7 @@ import { OnNavigationEventGenerator } from '../navigation-gen' const originalLocation = window.location -describe('OnNavigationEventGenerator', () => { +describe(OnNavigationEventGenerator, () => { let emitter: SignalEmitter let emitSpy: jest.SpiedFunction diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts index ccf8e8795..cb6ba9b39 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts @@ -6,25 +6,13 @@ import { SignalGenerator } from '../types' function getURLDifferences(url1: URL, url2: URL): ChangedProperties[] { const changed: ChangedProperties[] = [] - const propertiesToCompare: (keyof URL)[] = [ - 'href', - 'protocol', - 'host', - 'hostname', - 'port', - 'pathname', - 'search', - 'hash', - 'username', - 'password', - ] + const propertiesToCompare: (keyof URL)[] = ['pathname', 'search', 'hash'] for (const property of propertiesToCompare) { if (url1[property] !== url2[property]) { if (property === 'pathname') { changed.push('path') - } - if (['search', 'hash'].includes(property)) { + } else { changed.push(property as ChangedProperties) } } From 741d8bc44cc0f41cedf843524eb63bea5fff4721 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 10 Apr 2025 16:31:10 -0500 Subject: [PATCH 14/15] wip --- .../src/core/signal-generators/dom-gen/navigation-gen.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts index cb6ba9b39..26c0a463a 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts @@ -6,14 +6,14 @@ import { SignalGenerator } from '../types' function getURLDifferences(url1: URL, url2: URL): ChangedProperties[] { const changed: ChangedProperties[] = [] - const propertiesToCompare: (keyof URL)[] = ['pathname', 'search', 'hash'] + const propertiesToCompare = ['pathname', 'search', 'hash'] as const for (const property of propertiesToCompare) { if (url1[property] !== url2[property]) { if (property === 'pathname') { changed.push('path') } else { - changed.push(property as ChangedProperties) + changed.push(property) } } } From 6ec0221364789632dbc10ae4f1c470e0b95fcfc5 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Fri, 11 Apr 2025 12:20:04 -0500 Subject: [PATCH 15/15] add unsubscribe test --- .../dom-gen/__tests__/navigation-gen.test.ts | 36 +++++++++++++++++++ .../dom-gen/navigation-gen.ts | 4 +-- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts index b5bc7c371..4f1fd64d5 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/navigation-gen.test.ts @@ -125,4 +125,40 @@ describe(OnNavigationEventGenerator, () => { const lastCall = emitSpy.mock.lastCall![0].data as URLChangeNavigationData expect(lastCall.changedProperties).toEqual(['hash']) }) + + it('should stop emitting events after unsubscribe is called', () => { + const generator = new OnNavigationEventGenerator() + + const unsubscribe = generator.register(emitter) + + // Simulate a URL change + const newUrl = new URL(location.href) + newUrl.pathname = '/new-path' + setLocation({ + href: newUrl.href, + pathname: newUrl.pathname, + }) + + // Advance the timers to trigger the polling + jest.advanceTimersByTime(1000) + + // Ensure the event is emitted before unsubscribe + expect(emitSpy).toHaveBeenCalledTimes(2) + + // Unsubscribe the generator + unsubscribe() + + // Simulate another URL change + newUrl.pathname = '/another-path' + setLocation({ + href: newUrl.href, + pathname: newUrl.pathname, + }) + + // Advance the timers again + jest.advanceTimersByTime(1000) + + // Ensure no additional events are emitted after unsubscribe + expect(emitSpy).toHaveBeenCalledTimes(2) + }) }) diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts index 26c0a463a..bd5b133ac 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/navigation-gen.ts @@ -4,7 +4,7 @@ import { createNavigationSignal } from '../../../types/factories' import { SignalEmitter } from '../../emitter' import { SignalGenerator } from '../types' -function getURLDifferences(url1: URL, url2: URL): ChangedProperties[] { +function getChangedProperties(url1: URL, url2: URL): ChangedProperties[] { const changed: ChangedProperties[] = [] const propertiesToCompare = ['pathname', 'search', 'hash'] as const @@ -44,7 +44,7 @@ export class OnNavigationEventGenerator implements SignalGenerator { createNavigationSignal({ action: 'urlChange', prevUrl: previous.href, - changedProperties: getURLDifferences(current, previous), + changedProperties: getChangedProperties(current, previous), ...this.createCommonFields(), }) )