Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inverted comparison #29

Merged
merged 1 commit into from
Jan 29, 2019
Merged

Fix inverted comparison #29

merged 1 commit into from
Jan 29, 2019

Conversation

triblondon
Copy link
Contributor

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
Copy link

Coverage Status

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

1 similar comment
@coveralls
Copy link

Coverage Status

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

@grantila
Copy link
Owner

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

@grantila grantila merged commit 03d56a3 into grantila:master Jan 29, 2019
@triblondon
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants