Closed thomasheartman closed 1 year ago
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:
Thoughts?
Yes! Let's do that ✅
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 insrc/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:
null
orundefined
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 themetrics.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