search

grantila / fetch-h2

HTTP/1+2 Fetch API client for Node.js
MIT License
336 stars 16 forks source link

Fix inverted comparison #29

Closed triblondon closed 5 years ago

triblondon commented 5 years ago

I'm incredibly impressed with this module. Thanks so much for all your efforts and for releasing it for everyone to use. I think there is a bug here, but I'm not completely convinced so proposing the basic change and I'm happy to put more effort in if you can guide me to where it's needed.

My use case is a tool that can make highly configurable requests, and one of the configuration options is whether to use H1 or H2. I am then setting up a fetch-h2 context using:

new FetchContext({ httpProtocol: 'http1', httpsProtocols: [opts.useH2 ? 'http2' : 'http1'] })

So, I don't want fetch-h2 to negotiate a protocol for me, I want to pin to a particular one. However, I found that regardless of the value of the httpsProtocols option, fetch was always using H2 anyway. I believe the problem is that this conditional check is incorrectly inverted.

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 87.491% when pulling 476aba4f7177a8be37faf77c457acff2faa6cc9b on triblondon:patch-1 into be4d3d1f3590056fa319f721e429e9ab94c30b0f on grantila:master.

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 87.491% when pulling 476aba4f7177a8be37faf77c457acff2faa6cc9b on triblondon:patch-1 into be4d3d1f3590056fa319f721e429e9ab94c30b0f on grantila:master.

grantila commented 5 years ago

Thanks so much for the appreciation, and you're absolutely right, this is indeed supposed to be inverted.

triblondon commented 5 years ago

Thank you!