fix: use custom headers for metrics requests too

[thomasheartman]

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

[Tymek]

LGTM.

We don't need to improve it over the behavior we have in index.ts. I think not dealing with whitespace etc is OK, because custom headers is an optional, advanced use case.

I'm not sure about how we handle null / undefined headers. When I set
{ Accept: undefined } I would expect this to remove the default Accept: 'application/json' header.

[thomasheartman]

I'm not sure about how we handle null / undefined headers. When I set
{ Accept: undefined } I would expect this to remove the default Accept: 'application/json' header.

I think this is a valid point, and it's how I thought I would do it myself. At the same time, that's not how custom headers work when you fetch features. I think having them work differently would be very confusing, so I made sure that if you add a null/undefined header, then it doesn't affect existing headers (there's a test for that).

I'd be happy to change it, but that would probably require a major version bump because we should change the handling for toggle fetching too in that case.

So my suggestion is:

• Merge this now as it stands
• Document this behavior (I'll add that before merging)
• Optionally change how it works in v3

Thoughts?

[Tymek]

Yes! Let's do that ✅