fix: feature detection to check for browser #738
Conversation
Codecov Report
@@ Coverage Diff @@
## master #738 +/- ##
==========================================
+ Coverage 84.42% 84.51% +0.09%
==========================================
Files 18 17 -1
Lines 950 943 -7
Branches 210 210
==========================================
- Hits 802 797 -5
+ Misses 88 86 -2
Partials 60 60
Continue to review full report at Codecov.
|
bcoe
left a comment
There was a problem hiding this comment.
This seems reasonable to me. Is the only thing that changes between the browser and Node.js execution the certificate format? our hypothesis being that an environment like Jest will work with PEM?
| configure(opts: GaxiosOptions = {}): GaxiosOptions { | ||
| opts.headers = opts.headers || {}; | ||
| if (!isBrowser()) { | ||
| if (typeof window === 'undefined') { |
There was a problem hiding this comment.
Aye. In this case, it doesn't have an impact on which browser feature we're trying to use. It's just a naive check so we can try to send the right HTTP headers (I think).
|
@bcoe re: certificate formats, that's all bad bad bad. We basically need to have the same "feature detection check" there as well (not just "oh it's a browser, let's do JWK and not PEM", but "if this environment supports PEM, do PEM, if it supports JWK, do JWK, and there might be other formats to consider"). I think it's OK to leave it as is for now, until someone comes and claims that the existing logic does not work for them. |
Previously, we were using the precense of
windowto figure out if we were running in a browser. Turns out, it causes a lot of trouble, like this:googleapis/nodejs-storage#694 (comment)
We also see cases where
windowis injected in test environments for things likejest.This uses a feature detection style approach, where we specifically check for subtle crypto availability.