Skip to content

Adding Brotli Support - #598

Merged
bitinn merged 5 commits into
node-fetch:masterfrom
hlthi:master
Apr 26, 2019
Merged

bitinn merged 5 commits into
node-fetch:masterfrom
hlthi:master

Conversation

@hlthi

@hlthi hlthi commented Mar 31, 2019

Copy link
Copy Markdown
Contributor

@bitinn

bitinn commented Apr 25, 2019

Copy link
Copy Markdown
Collaborator

Thx for the PR, I will into merging it.

Just one request: could you change the tests to use skip on nodejs version without brotil support, instead of faking a pass by fallback to gzip?

@hlthi

hlthi commented Apr 25, 2019

Copy link
Copy Markdown
Contributor Author

Seems like "should ignore invalid headers" test gone wrong in Node 12 @bitinn .

  • gzip/skip Request has been completed.

@bitinn

bitinn commented Apr 25, 2019

Copy link
Copy Markdown
Collaborator

@hlthi I suspect node 12 has changed how they handle invalid headers, so this test now fails on test/server.js, do you have node 12 installed, can you verify the issue exists on your local machine?

The PR itself looks good, but I will need to fix the node 12 issue first.

@hlthi

hlthi commented Apr 25, 2019

Copy link
Copy Markdown
Contributor Author

Possible problems

  1. Switch default parser to llhttp

    Maybe new http parser dont know what to do invalid characters :(

  2. HTTP2 doesnt support invalid header

Header field-names must only contain one or more of the following ASCII characters: a-z, A-Z, 0-9, !, #, $, %, &, ', *, +, -, ., ^, _, (backtick), |, and ~.
`
But it isn't http2 so I eliminate that.

Debug err

{
  "bytesParsed": 171,
  "code": "HPE_INVALID_HEADER_TOKEN",
  "reason": "Invalid header value char"
}

Investigate in LLHTTP,

https://github.com/nodejs/llhttp/blob/7596c6456d38bfa8a60fddddf162212527462b45/src/llhttp/http.ts#L494-L499

LLHTTP waiting \r and \n ; but show \x07 and boom.

I try on macOS mojave with node 12

$ npm version | grep http_parser
http_parser: '2.8.0',

@bitinn

@bitinn

bitinn commented Apr 26, 2019

Copy link
Copy Markdown
Collaborator

thx for the PR, released as 2.4.0

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.

2 participants