fix: feature detection to check for browser #738
Conversation
googlebot
added
the
cla: yes
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.
|
| @@ -60,7 +58,7 @@ export class DefaultTransporter { | |||
| */ | |||
| configure(opts: GaxiosOptions = {}): GaxiosOptions { | |||
| opts.headers = opts.headers || {}; | |||
| if (!isBrowser()) { | |||
| if (typeof window === 'undefined') { | |||
There was a problem hiding this comment.
we're okay to keep this check the same?
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.