Added support for brotli ('br') content-encoding #406

danielgindi · 2020-07-13T19:12:50Z

https://medium.com/oyotech/how-brotli-compression-gave-us-37-latency-improvement-14d41e50fee4
https://caniuse.com/#feat=brotli

(Originally #403)

danielgindi · 2020-07-19T07:16:31Z

How are we doing?

UlisesGascon

LGTM @danielgindi!

Thanks fo the links attached to the PR. I will love to be able to try that compression in my next project 💪

danielgindi · 2020-08-04T20:21:41Z

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

dougwilson

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.

dougwilson · 2020-08-04T16:02:19Z

README.md

@@ -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.


I would think that the information about version support should be along with the br support docs, otherwise users may not see it.

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.

https://github.com/expressjs/body-parser/pull/406/files#diff-04c6e90faac2675aa89e2176d2eec7d8R74

Still, where in the README do you want this? There's no explicit section for br...

It is the spots where you added the br mentions that br is one of the options.

lib/read.js

danielgindi · 2020-08-08T20:12:28Z

@dougwilson Have you noticed that we suddenly have a "leak" in the older node versions in CI?
These happens without any apparent reason, and the change in the code was insignificant, only DRYed a throw statement.
That's why I disabled that notification to begin with.

danielgindi · 2020-12-19T16:03:12Z

@dougwilson Anything preventing this from being published?

dougwilson · 2020-12-19T16:46:23Z

package.json

@@ -45,7 +45,7 @@
  },
  "scripts": {
    "lint": "eslint --plugin markdown --ext js,md .",
-    "test": "mocha --require test/support/env --reporter spec --check-leaks --bail test/",
+    "test": "mocha --require test/support/env --reporter spec --check-leaks --global __core-js_shared__ --bail test/",


We really shouldn't be adding a global leak with our changes. Is there a way to fix the leak? Just adding it to ignore is not really the best solution.

@dougwilson It's from one of the libs used by either one of our existing dependencies, or mocha itself. Most probably by mocha itself.
It's a global leak introduced by json5 and by loadash in the old versions that we are referencing. It's for deduping the code-js libs.
We are not using these in our code, so I don't think we can do anything about it - except increase dependency versions, which will break old node support.

Interesting. Why doesn't the leak occur on master branch or other PRs, if that is the case?

Even in this PR, it happened on and off. Sometimes by changing a single line with no interesting sideeffects.
I don't know if you recall but we've added and removed this before.
Also this happens only on specific very old versions of node, where that core-js dependency was required.

Hm, strange. So that could just mean that these changes are causing it to be intermittent, not that it is intermittent without these changes. Of all the PRs and commits I push here, it has never happened, which is why it seems to point to the changes here somehow causing them to occur. It may help if maybe we approach this differently: can you just explain why it happens? For example, you say "happens only on specific very old versions of node, where that core-js dependency was required". So, which versions is that? Can we only add the exception to only those specific versions, then? You also say "except increase dependency versions". Increase which dependencies to which versions? If these are just test dependencies, we can change any of those to be whatever version is necessary for each different version of Node.js in our Travis CI file, like we are doing today.

I did not deep dive that much. I only recall it was all the 0.x versions except <=0.8.
I don't really want or care to start eliminating it line by line, as those node versions are not supported for years now, and I can't do these tests without pushing little changes again and again to your CI and wait for the results. You could look at the previous CI results and see what I mean. Or maybe the whole history of CI on this branch.

What I can say for sure is that this even happened before switching between 'brotli' in process.versions and 'createBrotliDecompress' in zlib, and stopped with this change.

@dougwilson More info: the users of this corejs quirk are babel (by requiring json5), and eslint.
eslint is the immediate suspect, as the modules it requires vary depending on the source code. An syntax combination somewhere could trigger another rule to be required, and then this leak will happen as a few of the rules require lodash which is the culprit.

ghost

** spam content removed **

ghost

** spam content removed **

ghost

** spam content removed **

ignlg · 2023-06-30T09:48:57Z

Is this feature stuck for some specific reason? Is there something that can be done to fix it?

rubiin

lgtm

danielgindi and others added 2 commits July 10, 2020 19:22

Added support for brotli ('br') content-encoding

180f373

Update README.md

466d618

dougwilson added enhancement pr labels Jul 13, 2020

UlisesGascon requested a review from dougwilson August 4, 2020 15:55

UlisesGascon approved these changes Aug 4, 2020

View reviewed changes

dougwilson suggested changes Aug 4, 2020

View reviewed changes

dougwilson reviewed Aug 4, 2020

View reviewed changes

lib/read.js Outdated Show resolved Hide resolved

DRYed error throwing (the one with encoding specified)

ff788ac

Updated brotli detection method

c08e899

Allow "__core-js_shared__" leak in mocha

d9ff5de

dougwilson suggested changes Dec 19, 2020

View reviewed changes

ghost approved these changes Aug 10, 2021

View reviewed changes

ghost approved these changes Aug 11, 2021

View reviewed changes

dougwilson force-pushed the master branch from 70560b1 to b356758 Compare January 28, 2022 05:13

dougwilson force-pushed the master branch from acc026f to b6bfabd Compare February 15, 2022 00:53

dougwilson force-pushed the master branch 3 times, most recently from 0ad1d88 to 2a2f471 Compare April 3, 2022 00:47

rubiin reviewed Aug 16, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for brotli ('br') content-encoding #406

Added support for brotli ('br') content-encoding #406

danielgindi commented Jul 13, 2020

danielgindi commented Jul 19, 2020

UlisesGascon left a comment •

edited

danielgindi commented Aug 4, 2020 •

edited

dougwilson left a comment

dougwilson Aug 4, 2020

danielgindi Aug 7, 2020

dougwilson Aug 7, 2020

danielgindi Aug 8, 2020

dougwilson Dec 19, 2020

danielgindi commented Aug 8, 2020

danielgindi commented Dec 19, 2020

dougwilson Dec 19, 2020

danielgindi Dec 19, 2020

dougwilson Dec 19, 2020

danielgindi Dec 19, 2020

dougwilson Dec 19, 2020

danielgindi Dec 19, 2020

danielgindi Dec 19, 2020

ghost left a comment •

edited by dougwilson

ghost left a comment •

edited by dougwilson

ghost left a comment •

edited by dougwilson

ignlg commented Jun 30, 2023

rubiin left a comment

Added support for brotli ('br') content-encoding #406

Are you sure you want to change the base?

Added support for brotli ('br') content-encoding #406

Conversation

danielgindi commented Jul 13, 2020

danielgindi commented Jul 19, 2020

UlisesGascon left a comment • edited

Choose a reason for hiding this comment

danielgindi commented Aug 4, 2020 • edited

dougwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielgindi commented Aug 8, 2020

danielgindi commented Dec 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment • edited by dougwilson

Choose a reason for hiding this comment

ghost left a comment • edited by dougwilson

Choose a reason for hiding this comment

ghost left a comment • edited by dougwilson

Choose a reason for hiding this comment

ignlg commented Jun 30, 2023

rubiin left a comment

Choose a reason for hiding this comment

UlisesGascon left a comment •

edited

danielgindi commented Aug 4, 2020 •

edited

ghost left a comment •

edited by dougwilson

ghost left a comment •

edited by dougwilson

ghost left a comment •

edited by dougwilson