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

add transparent http2 #406

Closed
wants to merge 1 commit into from
Closed

add transparent http2 #406

wants to merge 1 commit into from

Conversation

devsnek
Copy link

@devsnek devsnek commented Feb 22, 2018

mvp for http2 support (experimental and dangerous, don't complain if this deletes your cat pictures)

since http2 features and support have been gradually added through node 9.x, it would seem that this feature might need to be explicitly locked to node >= 10

todo

  • use Http2ClientSession for redirects within the same origin instead of creating a new one
  • tests

@gr2m
Copy link
Collaborator

gr2m commented Mar 1, 2018

@devsnek Hey Gus, I’m no maintainer of node-fetch but several other projects out there, some of which depend on node-fetch and I know some of the maintainers personally. I’m sure everyone appreciates that you take time to contribute. I would find your language unnecessary confrontational though. It would be reason enough to just ignore your pull request altogether. Open source tends to be a hostile place some times and your PR has lots of red flags.

A lot of people put a lot of time into node-fetch and I’m sure they have reasons for the preference of their code style. You are coming to their house and really should respect their rules ;) You could create a separate issue though with a friendly suggestion to change the code style and offer to help with the migration if the maintainers agree.

I would recommend you close your pull request, remove the eslint configuration, leave all the syntax as-is and only submit a minimal pull request with the changes required for http2. If you like, create an issue to suggest to make changes to the code style and wait for the maintainers think of it. And use kind language, it will help a lot to get maintainers attention, I can tell that from my own experience, both as maintainer and contributor

@devsnek
Copy link
Author

devsnek commented Mar 1, 2018

@gr2m I'm sorry if you found my words or actions abrasive in any way, I didn't intend it. I simply generated an eslint that kept the diff the smallest while keeping all the code in the library consistent. I also kept it in a separate commit. the issue i found was that i couldn't even figure out even whether to use spaces or indents in the project since different files use different styles.

@bitinn
Copy link
Collaborator

bitinn commented Mar 2, 2018

the mixed use of tab/space is unfortunately the status quo: some files are taken from nodejs core and thus use spaces while the main project was using tab.

on topic: http/2 support is too large a feature to be taken in a single PR (in browser it's transparent, nodejs it isn't, at least for now; Timothy might persuade nodejs to add a unified interface for http one day); adding eslint is not helping anyone to review this PR hence nobody did...

@devsnek
Copy link
Author

devsnek commented Mar 2, 2018

@bitinn this adds the feature as transparent using alpn negotiation. I've been messing around with it on my own request lib (I actually use node-fetch to test my lib 😄) and it seems quite stable. as for the eslint stuff I'll delete the commit

@jimmywarting
Copy link
Collaborator

Linting is put on hold for the time being #303
Have to agree with everything @gr2m says

@devsnek devsnek changed the title add http2 (and eslint in another commit) add transparent http2 Mar 2, 2018
@TimothyGu
Copy link
Collaborator

TimothyGu commented Mar 6, 2018

As I said in #342, we will need an equivalent of http.Agent to allow us to reuse Http2ClientSessions. This is kind of covered by

use Http2ClientSession for redirects within the same origin instead of creating a new one

only that the pool should be used not just for redirects but also for general purpose. A caveat however, is the connection in the connection pool must be keyed by BOTH origin AND the presence of credentials.

Thus, putting this on hold for now.

@devsnek devsnek closed this Apr 22, 2018
@devsnek devsnek mentioned this pull request Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants