fix: use custom headers for metrics requests too (#148)

This PR updates how custom headers work. Previously, any custom headers you added would only get added to feature get/post requests. Now we also add them to metrics requests. This is in line with what the documentation implies and what we think it's reasonable to expect. As such, I consider this a bug fix.

## About the changes

The most relevant changes are in the `src/metrics.ts`. In short, we perform the same kind of method here as in `src/index.ts` to enrich the standard headers with any custom headers that are provided.

We also take care to pass the custom headers from the client constructor to the metrics constructor.

### Tests

The change comes with the following tests:

1. Verify that the custom headers are passed on from the client constructor to the metrics constructor
2. Verify that custom headers ...
    - are sent
    - override preset headers, but only if they have a valid value
    - that are set to `null` or `undefined` don't get sent.

## Discussion points

At the time of writing, the current set of changes are essentially just a copy-paste of what we were doing in the `index.ts` file. While it's tempting to refactor this and extract it, that also seems like a bit of overkill to me. As far as I'm aware, there's only two copies, and they behave slightly differently (different headers), so I think the duplication is accidental more than intentional. We could parameterize it into a shared function (and I'm happy to do that), but I'm not sure it's worth it.

Second, we currently allow setting headers that are empty or all-whitespace strings. This is consistent with how the `index.ts` file does it. We can't change the index file, but we don't have to copy this behavior. Do we want to allow empty strings? I'm leaning towards yes because it's less surprising, but I'm open to hearing arguments.

Finally, I also considered applying the custom headers to the `fetch` parameter that we send in (by wrapping it in another function). This would allow us to not change anything inside the `metrics.ts` file at all, but I decided against it because I thought it might be harder to reason about and to test. Again, though, happy to do it if we think it's better.

---

Closes #142

## Commits

* fix: add initial tests for custom headers

* fix: add custom headers to metrics call

* fix: test that custom headers are passed along

* fix: pass custom headers to metrics

* fix: elaborate on ts-expect-error message

* docs: document how the sdk handles `null`/`undefined`

README.md

@@ -120,9 +120,9 @@ The Unleash SDK takes the following options:
 | fetch             | no | `window.fetch` or global `fetch` | Allows you to override the fetch implementation to use. Useful in Node.js environments where you can inject `node-fetch`                    | 
 | bootstrap         | no | `[]` | Allows you to bootstrap the cached feature toggle configuration.                                                                               | 
 | bootstrapOverride | no| `true` | Should the bootstrap automatically override cached data in the local-storage. Will only be used if bootstrap is not an empty array.     | 
-| headerName        | no| `Authorization` | Provides possiblity to specify custom header that is passed to Unleash / Unleash Proxy with the `clientKey` | 
-| customHeaders     | no| `{}` | Additional headers to use when making HTTP requests to the Unleash proxy. In case of name collisions with the default headers, the `customHeaders` value will be used. | 
-| impressionDataAll | no| `false` | Allows you to trigger "impression" events for **all** `getToggle` and `getVariant` invocations. This is particularly useful for "disabled" feature toggles that are not visible to frontend SDKs. | 
+| headerName        | no| `Authorization` | Which header the SDK should use to authorize with Unleash / Unleash Proxy. The header will be given the `clientKey` as its value. |
+| customHeaders     | no| `{}` | Additional headers to use when making HTTP requests to the Unleash proxy. In case of name collisions with the default headers, the `customHeaders` value will be used if it is not `null` or `undefined`. `customHeaders` values that are `null` or `undefined` will be ignored. |
+| impressionDataAll | no| `false` | Allows you to trigger "impression" events for **all** `getToggle` and `getVariant` invocations. This is particularly useful for "disabled" feature toggles that are not visible to frontend SDKs. |
 | environment | no | `default` | Sets the `environment` option of the [Unleash context](https://docs.getunleash.io/reference/unleash-context). This does **not** affect the SDK's [Unleash environment](https://docs.getunleash.io/reference/environments). |
 
 ### Listen for updates via the EventEmitter

src/index.test.ts

@@ -1172,9 +1172,20 @@ test('Should pass custom headers', async () => {
 
     jest.advanceTimersByTime(1001);
 
-    const request = getTypeSafeRequest(fetchMock);
+    const featureRequest = getTypeSafeRequest(fetchMock, 0);
 
-    expect(request.headers).toMatchObject({
+    expect(featureRequest.headers).toMatchObject({
+        customheader1: 'header1val',
+        customheader2: 'header2val',
+    });
+
+    const _ = client.isEnabled('count-metrics');
+    jest.advanceTimersByTime(2001);
+
+    const metricsRequest = getTypeSafeRequest(fetchMock, 1);
+
+    expect(metricsRequest.headers).toMatchObject({
+        customheader1: 'header1val',
         customheader2: 'header2val',
     });
 });

src/index.ts

@@ -185,6 +185,7 @@ export class UnleashClient extends TinyEmitter {
             clientKey,
             fetch,
             headerName,
+            customHeaders,
         });
     }
 

src/metrics.test.ts

@@ -62,7 +62,7 @@ test('should send metrics', async () => {
     expect(body.bucket.toggles.bar.no).toEqual(1);
 });
 
-test('should send metrics under custom header', async () => {
+test('should send metrics with custom auth header', async () => {
     const metrics = new Metrics({
         onError: console.error,
         appName: 'test',
@@ -145,3 +145,68 @@ test('should send metrics based on timer interval', async () => {
 
     expect(fetchMock.mock.calls.length).toEqual(3);
 });
+
+describe('Custom headers for metrics', () => {
+    const runMetrics = async (customHeaders: Record<string, string>) => {
+        const metrics = new Metrics({
+            onError: console.error,
+            appName: 'test',
+            metricsInterval: 5,
+            disableMetrics: false,
+            url: 'http://localhost:3000',
+            clientKey: '123',
+            fetch: fetchMock,
+            headerName: 'Authorization',
+            customHeaders,
+        });
+
+        metrics.count('foo', true);
+        await metrics.sendMetrics();
+
+        return getTypeSafeRequest(fetchMock);
+    };
+
+    test('Should apply any custom headers to the metrics request', async () => {
+        const customHeaders = {
+            'x-custom-header': '123',
+        };
+
+        const requestBody = await runMetrics(customHeaders);
+        expect(requestBody.headers).toMatchObject(customHeaders);
+    });
+
+    test('Custom headers should override preset headers', async () => {
+        const customHeaders = {
+            Authorization: 'definitely-not-the-client-key',
+        };
+
+        const requestBody = await runMetrics(customHeaders);
+        expect(requestBody.headers).toMatchObject(customHeaders);
+    });
+
+    test('Empty custom headers do not override preset headers on collision', async () => {
+        const customHeaders = {
+            Authorization: null,
+        };
+
+        // @ts-expect-error this shouldn't be allowed in TS, but there's
+        // nothing stopping you from doing it in JS.
+        const requestBody = await runMetrics(customHeaders);
+        expect(requestBody.headers).not.toMatchObject(customHeaders);
+    });
+
+    test.each([null, undefined])(
+        'Custom headers that are "%s" should not be sent',
+        async (emptyValue) => {
+            const customHeaders = {
+                'invalid-header': emptyValue,
+            };
+
+            // @ts-expect-error this shouldn't be allowed in TS, but there's
+            // nothing stopping you from doing it in JS.
+            const requestBody = await runMetrics(customHeaders);
+
+            expect(requestBody.headers).not.toMatchObject(customHeaders);
+        }
+    );
+});

src/metrics.ts

@@ -1,5 +1,7 @@
 // Simplified version of: https://github.com/Unleash/unleash-client-node/blob/main/src/metrics.ts
 
+import { notNullOrUndefined } from './util';
+
 export interface MetricsOptions {
     onError: OnError;
     appName: string;
@@ -9,6 +11,7 @@ export interface MetricsOptions {
     clientKey: string;
     fetch: any;
     headerName: string;
+    customHeaders?: Record<string, string>;
 }
 
 interface Bucket {
@@ -36,6 +39,7 @@ export default class Metrics {
     private timer: any;
     private fetch: any;
     private headerName: string;
+    private customHeaders: Record<string, string>;
 
     constructor({
         onError,
@@ -46,6 +50,7 @@ export default class Metrics {
         clientKey,
         fetch,
         headerName,
+        customHeaders = {},
     }: MetricsOptions) {
         this.onError = onError;
         this.disabled = disableMetrics;
@@ -56,6 +61,7 @@ export default class Metrics {
         this.bucket = this.createEmptyBucket();
         this.fetch = fetch;
         this.headerName = headerName;
+        this.customHeaders = customHeaders;
     }
 
     public start() {
@@ -90,6 +96,20 @@ export default class Metrics {
         };
     }
 
+    private getHeaders() {
+        const headers = {
+            [this.headerName]: this.clientKey,
+            Accept: 'application/json',
+            'Content-Type': 'application/json',
+        };
+
+        Object.entries(this.customHeaders)
+            .filter(notNullOrUndefined)
+            .forEach(([name, value]) => (headers[name] = value));
+
+        return headers;
+    }
+
     public async sendMetrics(): Promise<void> {
         /* istanbul ignore next if */
 
@@ -104,11 +124,7 @@ export default class Metrics {
             await this.fetch(url, {
                 cache: 'no-cache',
                 method: 'POST',
-                headers: {
-                    [this.headerName]: this.clientKey,
-                    Accept: 'application/json',
-                    'Content-Type': 'application/json',
-                },
+                headers: this.getHeaders(),
                 body: JSON.stringify(payload),
             });
         } catch (e) {