Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Added support for brotli ('br') content-encoding #406
Conversation
|
How are we doing? |
|
LGTM @danielgindi! Thanks fo the links attached to the PR. I will love to be able to try that compression in my next project |
Yeah this brotli is truly amazing :) Btw we have a PR in the compression repo, to allow responses to be encoded too. It's two sides of the same coin |
|
I moved over one comment that was still outstanding from the first repo and a general comment to make sure our detection makes sense from the noode.js project. |
| @@ -16,6 +16,8 @@ before trusting. For example, `req.body.foo.toString()` may fail in multiple | |||
| ways, for example the `foo` property may not be there or may not be a string, | |||
| and `toString` may not be a function and instead a string or other user input. | |||
|
|
|||
| **Note** Brotli is supported only since Node.js versions v11.7.0 and v10.16.0. | |||
dougwilson
Aug 4, 2020
Member
I would think that the information about version support should be along with the br support docs, otherwise users may not see it.
I would think that the information about version support should be along with the br support docs, otherwise users may not see it.
danielgindi
Aug 7, 2020
Author
I think you might be confusing the compression repo. In this one there's no br support section. As there's nothing to configure, it's just decoding.
I think you might be confusing the compression repo. In this one there's no br support section. As there's nothing to configure, it's just decoding.
danielgindi
Aug 8, 2020
Author
Still, where in the README do you want this? There's no explicit section for br...
Still, where in the README do you want this? There's no explicit section for br...
| * @const | ||
| * whether current node version has brotli support | ||
| */ | ||
| var hasBrotliSupport = 'brotli' in process.versions |
dougwilson
Aug 4, 2020
Member
This check feels a bit disconnected from the actual usage, and Node.js does not really explain the expectations around this structure. I went ahead and asked what Node.js thought we should do at nodejs/help#2897
This check feels a bit disconnected from the actual usage, and Node.js does not really explain the expectations around this structure. I went ahead and asked what Node.js thought we should do at nodejs/help#2897
danielgindi
Aug 5, 2020
Author
Seems like they don't have an answer yet 😂
Btw you know that you can commit changes to a branch of a PR?
Seems like they don't have an answer yet
Btw you know that you can commit changes to a branch of a PR?
dougwilson
Aug 7, 2020
Member
Looks like there is a question on there @danielgindi . Can you answer their question there? Also, what changes do you want me to make? I'd be happy to make them.
Looks like there is a question on there @danielgindi . Can you answer their question there? Also, what changes do you want me to make? I'd be happy to make them.
danielgindi
Aug 8, 2020
Author
It's a question for you actually. It's about semantics. What exactly do you feel is right to detect there? I don't have a strong feeling towards either way.
And - any changes you feel are right :-)
It's a question for you actually. It's about semantics. What exactly do you feel is right to detect there? I don't have a strong feeling towards either way.
And - any changes you feel are right :-)
|
@dougwilson Have you noticed that we suddenly have a "leak" in the older node versions in CI? |
expressjs/compression#172
https://medium.com/oyotech/how-brotli-compression-gave-us-37-latency-improvement-14d41e50fee4
https://caniuse.com/#feat=brotli
(Originally #403)