diff --git a/.changeset/spotty-apes-matter.md b/.changeset/spotty-apes-matter.md new file mode 100644 index 000000000..f9227ee37 --- /dev/null +++ b/.changeset/spotty-apes-matter.md @@ -0,0 +1,6 @@ +--- +'@segment/analytics-signals': minor +'@segment/analytics-signals-runtime': minor +--- + +Add HTTPMethods. Do not gate requests based on content-type. diff --git a/packages/signals/signals-integration-tests/src/tests/signals-vanilla/network-signals-fetch.test.ts b/packages/signals/signals-integration-tests/src/tests/signals-vanilla/network-signals-fetch.test.ts index 9470866d4..6c833719e 100644 --- a/packages/signals/signals-integration-tests/src/tests/signals-vanilla/network-signals-fetch.test.ts +++ b/packages/signals/signals-integration-tests/src/tests/signals-vanilla/network-signals-fetch.test.ts @@ -10,7 +10,7 @@ test.describe('network signals - fetch', () => { indexPage = await new IndexPage().loadAndWait(page, basicEdgeFn) }) - test('should not emit non-json requests', async () => { + test('should emit non-json requests but not include the data object', async () => { await indexPage.network.mockTestRoute('http://localhost/upload', { body: JSON.stringify({ foo: 'test' }), }) @@ -26,10 +26,16 @@ test.describe('network signals - fetch', () => { (el) => el.properties!.data.action === 'request' ) - expect(requests).toHaveLength(0) + expect(requests[0].properties!.data).toMatchObject({ + action: 'request', + contentType: 'multipart/form-data', + data: null, + method: 'POST', + url: 'http://localhost/upload', + }) }) - test('should try to parse the body of text/plain requests', async () => { + test('should try to parse the body of any requests with string in the body', async () => { await indexPage.network.mockTestRoute('http://localhost/test', { body: JSON.stringify({ foo: 'test' }), contentType: 'application/json', @@ -251,19 +257,4 @@ test.describe('network signals - fetch', () => { expect(responses).toHaveLength(1) }) }) - - test('not emit response errors if there is no corresponding request, even if correct content type', async () => { - await indexPage.network.mockTestRoute('http://localhost/test', { - body: JSON.stringify({ foo: 'test' }), - contentType: 'application/json', - status: 400, - }) - - await indexPage.network.makeFileUploadRequest('http://localhost/test') - - await indexPage.waitForSignalsApiFlush() - - const networkEvents = indexPage.signalsAPI.getEvents('network') - expect(networkEvents).toHaveLength(0) - }) }) 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 4f0554f22..6380011cb 100644 --- a/packages/signals/signals-runtime/src/web/web-signals-types.ts +++ b/packages/signals/signals-runtime/src/web/web-signals-types.ts @@ -75,7 +75,7 @@ interface BaseNetworkData { interface NetworkRequestData extends BaseNetworkData { action: 'request' url: string - method: string + method: HTTPMethod } interface NetworkResponseData extends BaseNetworkData { @@ -85,6 +85,15 @@ interface NetworkResponseData extends BaseNetworkData { ok: boolean } +export type HTTPMethod = + | 'GET' + | 'POST' + | 'PUT' + | 'DELETE' + | 'PATCH' + | 'HEAD' + | 'OPTIONS' + export type NetworkData = NetworkRequestData | NetworkResponseData export type NetworkSignal = RawSignal<'network', NetworkData> diff --git a/packages/signals/signals/src/core/client/__tests__/client.test.ts b/packages/signals/signals/src/core/client/__tests__/client.test.ts index f619a2fe3..824e040f7 100644 --- a/packages/signals/signals/src/core/client/__tests__/client.test.ts +++ b/packages/signals/signals/src/core/client/__tests__/client.test.ts @@ -49,7 +49,7 @@ describe(SignalsIngestClient, () => { data: { hello: 'how are you', }, - method: 'post', + method: 'POST', url: 'http://foo.com', }, }) @@ -63,7 +63,7 @@ describe(SignalsIngestClient, () => { "data": { "hello": "XXX", }, - "method": "post", + "method": "POST", "url": "http://foo.com", } `) diff --git a/packages/signals/signals/src/core/client/__tests__/redact.test.ts b/packages/signals/signals/src/core/client/__tests__/redact.test.ts index a09b1db9e..7ffe5c626 100644 --- a/packages/signals/signals/src/core/client/__tests__/redact.test.ts +++ b/packages/signals/signals/src/core/client/__tests__/redact.test.ts @@ -93,7 +93,7 @@ describe(redactSignalData, () => { { contentType: 'application/json', action: 'request', - method: 'post', + method: 'POST', url: 'http://foo.com', data: { name: 'John Doe', age: 30 }, }, @@ -103,7 +103,7 @@ describe(redactSignalData, () => { { contentType: 'application/json', action: 'request', - method: 'post', + method: 'POST', url: 'http://foo.com', data: { name: 'XXX', age: 999 }, }, diff --git a/packages/signals/signals/src/core/signal-generators/network-gen/__tests__/network-generator.test.ts b/packages/signals/signals/src/core/signal-generators/network-gen/__tests__/network-generator.test.ts index 36815f63b..be53ba44c 100644 --- a/packages/signals/signals/src/core/signal-generators/network-gen/__tests__/network-generator.test.ts +++ b/packages/signals/signals/src/core/signal-generators/network-gen/__tests__/network-generator.test.ts @@ -7,6 +7,7 @@ import { SignalEmitter } from '../../../emitter' import { Response } from 'node-fetch' import { sleep } from '@internal/test-helpers' import { setLocation } from '../../../../test-helpers/set-location' +import { NetworkSignal } from '@segment/analytics-signals-runtime' // xhr tests are in integration tests describe(NetworkGenerator, () => { @@ -126,7 +127,7 @@ describe(NetworkGenerator, () => { unregister() }) - it('should emit response signal but not request signal if content-type for request is not recognized ', async () => { + it('should emit response signal and request signal, regardless of content type', async () => { const mockEmitter = { emit: jest.fn() } const networkGenerator = new TestNetworkGenerator() const unregister = networkGenerator.register( @@ -134,35 +135,46 @@ describe(NetworkGenerator, () => { ) await window.fetch(`/api`, { - method: 'GET', + method: 'POST', headers: { 'content-type': 'text/html' }, body: 'hello world', }) await sleep(100) - expect(mockEmitter.emit.mock.calls).toMatchInlineSnapshot(` + expect( + mockEmitter.emit.mock.calls.flatMap((call) => + call.map((s: NetworkSignal) => s.data.action) + ) + ).toMatchInlineSnapshot(` [ - [ - { - "data": { - "action": "response", - "contentType": "application/json", - "data": { - "data": "test", - }, - "ok": true, - "status": 200, - "url": "http://localhost/api", - }, - "metadata": { - "filters": { - "allowed": [], - "disallowed": [], - }, - }, - "type": "network", - }, - ], + "request", + "response", + ] + `) + + unregister() + }) + + it('should emit response signal and request signal if no content-type', async () => { + const mockEmitter = { emit: jest.fn() } + const networkGenerator = new TestNetworkGenerator() + const unregister = networkGenerator.register( + mockEmitter as unknown as SignalEmitter + ) + + await window.fetch(`/some-data?foo=123`, { + method: 'GET', + }) + + await sleep(100) + expect( + mockEmitter.emit.mock.calls.flatMap((call) => + call.map((s: NetworkSignal) => s.data.action) + ) + ).toMatchInlineSnapshot(` + [ + "request", + "response", ] `) diff --git a/packages/signals/signals/src/core/signal-generators/network-gen/index.ts b/packages/signals/signals/src/core/signal-generators/network-gen/index.ts index 8b0de8b5f..9e4b88b19 100644 --- a/packages/signals/signals/src/core/signal-generators/network-gen/index.ts +++ b/packages/signals/signals/src/core/signal-generators/network-gen/index.ts @@ -12,11 +12,7 @@ import { NetworkRequestHandler, NetworkResponseHandler, } from './network-interceptor' -import { - containsJSONContentType, - containsJSONParseableContentType, - tryJSONParse, -} from './helpers' +import { containsJSONContentType, tryJSONParse } from './helpers' export type NetworkSettingsConfigSettings = Pick< SignalsSettingsConfig, @@ -58,10 +54,6 @@ export class NetworkGenerator implements SignalGenerator { }) const handleRequest: NetworkRequestHandler = (rq) => { - if (!containsJSONParseableContentType(rq.headers)) { - return - } - if (!rq.url || !this.filter.isAllowed(rq.url)) { return } diff --git a/packages/signals/signals/src/core/signal-generators/network-gen/network-interceptor.ts b/packages/signals/signals/src/core/signal-generators/network-gen/network-interceptor.ts index 0390dd330..ec85592a7 100644 --- a/packages/signals/signals/src/core/signal-generators/network-gen/network-interceptor.ts +++ b/packages/signals/signals/src/core/signal-generators/network-gen/network-interceptor.ts @@ -10,9 +10,22 @@ import { let origFetch: typeof window.fetch let origXMLHttpRequest: typeof XMLHttpRequest +export type HTTPMethod = + | 'GET' + | 'POST' + | 'PUT' + | 'DELETE' + | 'PATCH' + | 'HEAD' + | 'OPTIONS' + +const toHTTPMethod = (method: string): HTTPMethod => { + return method.toUpperCase() as HTTPMethod +} + export interface NetworkInterceptorRequest { url: string - method: string + method: HTTPMethod contentType: string | undefined body: string | undefined headers: Headers | undefined @@ -42,7 +55,7 @@ const createInterceptorRequest = ({ }: { url: URL | string body: string | undefined - method: string + method: HTTPMethod headers: Headers | undefined id: string }): NetworkInterceptorRequest => ({ @@ -130,7 +143,7 @@ export class NetworkInterceptor { createInterceptorRequest({ url: normalizedURL, body: typeof options?.body == 'string' ? options.body : undefined, - method: options?.method ?? 'GET', + method: options?.method ? toHTTPMethod(options.method) : 'GET', headers, id, }) @@ -185,7 +198,7 @@ export class NetworkInterceptor { const OrigXMLHttpRequest = window.XMLHttpRequest class InterceptedXMLHttpRequest extends OrigXMLHttpRequest { _reqURL?: string - _reqMethod?: string + _reqMethod?: HTTPMethod _reqBody?: XMLHttpRequestBodyInit _reqHeaders?: Headers _reqId = createRequestId() @@ -263,7 +276,7 @@ export class NetworkInterceptor { const [method, url] = args try { this._reqURL = url.toString() - this._reqMethod = method + this._reqMethod = toHTTPMethod(method) } catch (err) { console.log('Error handling request (open)', err) } diff --git a/packages/signals/signals/src/types/__tests__/create-network-signal.test.ts b/packages/signals/signals/src/types/__tests__/create-network-signal.test.ts index baa21c407..374f2f960 100644 --- a/packages/signals/signals/src/types/__tests__/create-network-signal.test.ts +++ b/packages/signals/signals/src/types/__tests__/create-network-signal.test.ts @@ -21,7 +21,7 @@ describe(createNetworkSignal, () => { const data: NetworkData = { action: 'request', url: 'http://example.com', - method: 'post', + method: 'POST', data: { key: 'value' }, contentType: 'application/json', } diff --git a/packages/signals/signals/src/types/factories.ts b/packages/signals/signals/src/types/factories.ts index 407b1ad87..9ce65303e 100644 --- a/packages/signals/signals/src/types/factories.ts +++ b/packages/signals/signals/src/types/factories.ts @@ -65,9 +65,6 @@ export const createNetworkSignal = ( data: { ...data, url: normalizeUrl(data.url), - ...(data.action === 'request' - ? { method: data.method.toUpperCase() } - : {}), }, metadata: metadata, }