Fix runtime errors on signals, allow plain/text in response (#1177)

Author: silesky

.changeset/little-chefs-hunt.md

@@ -0,0 +1,8 @@
+---
+'@segment/analytics-signals': minor
+---
+- Fix runtime errors for submit
+- Add better form submit data 
+- Loosen content-type to parse text/plain
+- Tweak disallow list
+- Add labels

packages/signals/signals-integration-tests/src/helpers/network-utils.ts

@@ -57,13 +57,38 @@ export class PageNetworkUtils {
     await req
     return responseBody
   }
+  async makeFileUploadRequest(url: string) {
+    let normalizeUrl = url
+    if (url.startsWith('/')) {
+      normalizeUrl = new URL(url, this.page.url()).href
+    }
+    const req = this.page.waitForResponse(normalizeUrl ?? url, {
+      timeout: this.defaultResponseTimeout,
+    })
+    await this.page.evaluate((_url) => {
+      const formData = new FormData()
+      const file = new File(['file content'], 'test.txt', {
+        type: 'text/plain',
+      })
+      formData.append('file', file)
+      return fetch(_url, {
+        method: 'POST',
+        body: formData,
+        headers: {
+          'Content-Type': 'multipart/form-data',
+        },
+      })
+    }, normalizeUrl)
+    await req
+  }
   /**
    * Make a fetch call in the page context. By default it will POST a JSON object with {foo: 'bar'}
    */
   async makeFetchCall(
     url = this.defaultTestApiURL,
     request: Partial<RequestInit> & {
       contentType?: string
+      blob?: boolean
     } = {}
   ): Promise<any> {
     let normalizeUrl = url

packages/signals/signals-integration-tests/src/tests/signals-vanilla/basic.test.ts

@@ -164,10 +164,11 @@ test('navigation signals', async ({ page }) => {
 
   // navigate to a new hash
   {
+    const flush = indexPage.waitForSignalsApiFlush()
     await page.evaluate(() => {
       window.location.hash = '#foo'
     })
-    await indexPage.waitForSignalsApiFlush()
+    await flush
     expect(indexPage.signalsAPI.getEvents()).toHaveLength(2)
     const ev = indexPage.signalsAPI.lastEvent('navigation')
     expect(ev.properties).toMatchObject({

packages/signals/signals-integration-tests/src/tests/signals-vanilla/change-input.test.ts

@@ -39,6 +39,10 @@ test('Collecting signals whenever a user enters text input', async ({
       labels: [
         {
           textContent: 'Name:',
+          id: '',
+          attributes: {
+            for: 'name',
+          },
         },
       ],
       name: 'name',

packages/signals/signals-integration-tests/src/tests/signals-vanilla/network-signals-fetch.test.ts

@@ -10,12 +10,60 @@ test.describe('network signals - fetch', () => {
     indexPage = await new IndexPage().loadAndWait(page, basicEdgeFn)
   })
 
-  test('should not emit anything if neither request nor response are json', async () => {
+  test('should not emit non-json requests', async () => {
+    await indexPage.network.mockTestRoute('http://localhost/upload', {
+      body: JSON.stringify({ foo: 'test' }),
+    })
+
+    await indexPage.network.makeFileUploadRequest('http://localhost/upload')
+
+    await indexPage.waitForSignalsApiFlush()
+
+    const networkEvents = indexPage.signalsAPI.getEvents('network')
+
+    // Check the request
+    const requests = networkEvents.filter(
+      (el) => el.properties!.data.action === 'request'
+    )
+
+    expect(requests).toHaveLength(0)
+  })
+
+  test('should try to parse the body of text/plain requests', async () => {
     await indexPage.network.mockTestRoute('http://localhost/test', {
-      body: 'hello',
+      body: JSON.stringify({ foo: 'test' }),
+      contentType: 'application/json',
+    })
+
+    await indexPage.network.makeFetchCall('http://localhost/test', {
+      method: 'POST',
+      body: JSON.stringify({ key: 'value' }),
       contentType: 'text/plain',
     })
 
+    await indexPage.waitForSignalsApiFlush()
+
+    const networkEvents = indexPage.signalsAPI.getEvents('network')
+
+    // Check the request
+    const requests = networkEvents.filter(
+      (el) => el.properties!.data.action === 'request'
+    )
+
+    expect(requests).toHaveLength(1)
+    expect(requests[0].properties!.data).toMatchObject({
+      action: 'request',
+      url: 'http://localhost/test',
+      data: { key: 'value' },
+    })
+  })
+
+  test('should send the raw string if the request body cannot be parsed as json', async () => {
+    await indexPage.network.mockTestRoute('http://localhost/test', {
+      body: JSON.stringify({ foo: 'test' }),
+      contentType: 'application/json',
+    })
+
     await indexPage.network.makeFetchCall('http://localhost/test', {
       method: 'POST',
       body: 'hello world',
@@ -26,7 +74,17 @@ test.describe('network signals - fetch', () => {
 
     const networkEvents = indexPage.signalsAPI.getEvents('network')
 
-    expect(networkEvents).toHaveLength(0)
+    // Check the request
+    const requests = networkEvents.filter(
+      (el) => el.properties!.data.action === 'request'
+    )
+
+    expect(requests).toHaveLength(1)
+    expect(requests[0].properties!.data).toMatchObject({
+      action: 'request',
+      url: 'http://localhost/test',
+      data: 'hello world',
+    })
   })
 
   test('can make a basic json request / not break regular fetch calls', async () => {
@@ -56,6 +114,7 @@ test.describe('network signals - fetch', () => {
     expect(requests).toHaveLength(1)
     expect(requests[0].properties!.data).toEqual({
       action: 'request',
+      contentType: 'application/json',
       url: 'http://localhost/test',
       method: 'POST',
       data: { key: 'value' },
@@ -67,6 +126,7 @@ test.describe('network signals - fetch', () => {
     expect(responses).toHaveLength(1)
     expect(responses[0].properties!.data).toEqual({
       action: 'response',
+      contentType: 'application/json',
       url: 'http://localhost/test',
       data: { foo: 'test' },
       status: 200,
@@ -112,39 +172,6 @@ test.describe('network signals - fetch', () => {
     })
   })
 
-  test('should emit response but not request if request content-type is not json but response is', async () => {
-    await indexPage.network.mockTestRoute('http://localhost/test', {
-      body: JSON.stringify({ foo: 'test' }),
-      contentType: 'application/json',
-    })
-
-    await indexPage.network.makeFetchCall('http://localhost/test', {
-      method: 'POST',
-      body: 'hello world',
-      contentType: 'text/plain',
-    })
-
-    await indexPage.waitForSignalsApiFlush()
-
-    const networkEvents = indexPage.signalsAPI.getEvents('network')
-
-    const responses = networkEvents.filter(
-      (el) => el.properties!.data.action === 'response'
-    )
-    expect(responses).toHaveLength(1)
-    expect(responses[0].properties!.data).toMatchObject({
-      action: 'response',
-      url: 'http://localhost/test',
-      data: { foo: 'test' },
-    })
-
-    // Ensure no request was captured
-    const requests = networkEvents.filter(
-      (el) => el.properties!.data.action === 'request'
-    )
-    expect(requests).toHaveLength(0)
-  })
-
   test.describe('errors', () => {
     test('will handle a json error response', async () => {
       await indexPage.network.mockTestRoute('http://localhost/test', {
@@ -232,11 +259,7 @@ test.describe('network signals - fetch', () => {
       status: 400,
     })
 
-    await indexPage.network.makeFetchCall('http://localhost/test', {
-      method: 'POST',
-      body: 'hello world',
-      contentType: 'text/plain',
-    })
+    await indexPage.network.makeFileUploadRequest('http://localhost/test')
 
     await indexPage.waitForSignalsApiFlush()
 

packages/signals/signals-integration-tests/src/tests/signals-vanilla/network-signals-xhr.test.ts

@@ -1,6 +1,5 @@
 import { test, expect } from '@playwright/test'
 import { IndexPage } from './index-page'
-import { sleep } from '@segment/analytics-core'
 
 const basicEdgeFn = `const processSignal = (signal) => {}`
 
@@ -10,27 +9,6 @@ test.describe('network signals - XHR', () => {
   test.beforeEach(async ({ page }) => {
     indexPage = await new IndexPage().loadAndWait(page, basicEdgeFn)
   })
-  test('should not emit anything if neither request nor response are json', async () => {
-    await indexPage.network.mockTestRoute('http://localhost/test', {
-      body: 'hello',
-      contentType: 'text/plain',
-    })
-
-    await indexPage.network.makeXHRCall('http://localhost/test', {
-      method: 'POST',
-      body: 'hello world',
-      contentType: 'text/plain',
-      responseType: 'text',
-    })
-
-    // Wait for the signals to be flushed
-    await sleep(300)
-
-    const networkEvents = indexPage.signalsAPI.getEvents('network')
-
-    // Ensure no request or response was captured
-    expect(networkEvents).toHaveLength(0)
-  })
 
   test('basic json request / not break XHR', async () => {
     await indexPage.network.mockTestRoute('http://localhost/test', {
@@ -111,7 +89,7 @@ test.describe('network signals - XHR', () => {
     })
   })
 
-  test('should emit response but not request if request content-type is not json but response is', async () => {
+  test('should emit request content type, even if not json', async () => {
     await indexPage.network.mockTestRoute('http://localhost/test', {
       body: JSON.stringify({ foo: 'test' }),
       contentType: 'application/json',
@@ -128,7 +106,18 @@ test.describe('network signals - XHR', () => {
 
     const networkEvents = await indexPage.signalsAPI.waitForEvents(1, 'network')
 
-    // Check the response (only response should be captured)
+    // ensure request
+    const requests = networkEvents.filter(
+      (el) => el.properties!.data.action === 'request'
+    )
+    expect(requests).toHaveLength(1)
+    expect(requests[0].properties!.data).toMatchObject({
+      action: 'request',
+      url: 'http://localhost/test',
+      data: 'hello world',
+    })
+
+    // Check the response
     const responses = networkEvents.filter(
       (el) => el.properties!.data.action === 'response'
     )
@@ -138,12 +127,6 @@ test.describe('network signals - XHR', () => {
       url: 'http://localhost/test',
       data: { foo: 'test' },
     })
-
-    // Ensure no request was captured
-    const requests = networkEvents.filter(
-      (el) => el.properties!.data.action === 'request'
-    )
-    expect(requests).toHaveLength(0)
   })
 
   test('should parse response if responseType is set to json but response header does not contain application/json', async () => {

packages/signals/signals-integration-tests/src/tests/signals-vanilla/signals-ingestion.test.ts

@@ -8,12 +8,11 @@ const basicEdgeFn = `const processSignal = (signal) => {}`
 test('ingestion not enabled -> will not send the signal', async ({ page }) => {
   await indexPage.loadAndWait(page, basicEdgeFn, {
     enableSignalsIngestion: false,
+    flushAt: 1,
   })
-
   await indexPage.fillNameInput('John Doe')
-  await indexPage.waitForSignalsApiFlush().catch(() => {
-    expect(true).toBe(true)
-  })
+  await page.waitForTimeout(100)
+  expect(indexPage.signalsAPI.getEvents('interaction')).toHaveLength(0)
 })
 
 test('ingestion enabled -> will send the signal', async ({ page }) => {

packages/signals/signals-runtime/README.md

@@ -1,10 +1,10 @@
 # @segment/analytics-signals-runtime
-Encapsults Signals runtime functionality, in order to share logic between the signals plugins for browser and mobile.
+Encapsulates Signals runtime functionality, in order to share logic between the signals plugins for browser and mobile.
 
 ### Development
 `yarn build` generate the following artifacts:
 | Generated File(s) | Description | 
 |--------|-------------|
 | `/dist/runtime/index.[platform].js`, `/[platform]/get-runtime-code.generated.js` | Exposes `globalThis.Signals` and constants (e.g. `SignalType`), either through the script tag or in the mobile JS engine | 
 | `/dist/editor/[platform]-editor.d.ts.txt` | Type definitions for monaco editor on app.segment.com 
-| `/dist/esm/index.js` | Entry point for `@segment/analytics-signals` and the segment app, that want to consume the types / runtime code. | 
+| `/dist/esm/index.js` | Entry point for `@segment/analytics-signals` and the segment app, for consumers that want to consume the types / runtime code as an npm package. | 

packages/signals/signals-runtime/src/test-helpers/mocks/mock-signal-types-web.ts

@@ -38,6 +38,7 @@ export const mockNetworkSignal: NetworkSignal = {
   type: 'network',
   data: {
     action: 'request',
+    contentType: 'application/json',
     url: 'https://api.example.com/data',
     method: 'GET',
     data: { key: 'value' },

packages/signals/signals-runtime/src/web/web-signals-types.ts

@@ -21,7 +21,8 @@ type ClickData = {
 
 type SubmitData = {
   eventType: 'submit'
-  submitter: SerializedTarget
+  submitter?: SerializedTarget
+  target: SerializedTarget
 }
 
 type ChangeData = {
@@ -68,6 +69,7 @@ interface BaseNetworkData {
   action: string
   url: string
   data: JSONValue
+  contentType: string
 }
 
 interface NetworkRequestData extends BaseNetworkData {