brotli: add brotli support by Hackzzila · Pull Request #20458 · nodejs/node · GitHub

Fixes: #18964

Adds brotli support to core. The brotli module must be enabled with --expose-brotli. The api, and most of the source code, is almost identical to the zlib module.

There are a few things that need polishing but I thought I would open this up for discussion.

/cc @MylesBorins

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

The api, and most of the source code, is almost identical to the zlib module.

Do you think it would be possible to work on re-using as much code as possible? I think that would be a very good idea if we can make it work (because it’s going to happen eventually anyway).

without overstepping this pr too much, maybe we can take this opportunity to think of a new way to group our compression stuff, both in terms of source and namespace (require('compression/brotli') or something)

As per Introducing New Modules policy, beware:

https://www.npmjs.com/package/brotli

cc @devongovett

As per Introducing New Modules policy, beware:

I managed to miss that, I can talk to the author but maybe this could be moved under a scope as brought up in nodejs/TSC#389

hey guys, I'm the owner of brotli on npm. That package is a hand coded decoder + emscripten ported encoder which both work in the browser as well as node. I'd be happy to make the package's API compatible with whatever API node decides on and use it as a browserify-style port so it can be used in the browser. WDYT?

Doc part LGTM with some nits. Sorry for a big bunch of them)

And thank you!

Should we add metadata?

<!-- YAML
 added: REPLACEME
-->

I didn't know what I should put there so I just removed those. Should they just be set to like v10.x.x? Or I guess REPLACEME works too :)

also with our pattern i think it should be --experimental-brotli

If I recall correctly, REPLACEME has some part in build automation as a do-not-forget-to-replace guard)

brotli APIs -> `brotli` APIs?

http request -> HTTP request or `http` request?

balace ->balance?

accept -> accepts?

If it can be a string, then buffer -> data here and below?

Promise -> `Promise` or even {Promise} (will be linkified)?
Ditto below.

Compress a chunk of data with [Compress][].

@vsemozhetbyt a few of the things you mentioned also apply to the zlib docs, those should probably be looked over sometime as well.

[options]?

We usually sort bottom references in ASCII order, so this should go before the [`brotli/encode.h`]:.

References below also need some resorting.

Don't know how I managed to mess up sorting 😆

@vsemozhetbyt a few of the things you mentioned also apply to the zlib docs, those should probably be looked over sometime as well.

Yeah, our docs are far from ideal, but we are working on them)

I will go ahead and create brotli label)

I believe «Note: » prefixes were removed from the docs earlier in #18592.
Are those needed here?

@vsemozhetbyt thanks for the docs review! Hopefully I have fixed everything now.

May be const now)

If only eslint for vscode checked markdown too.

Can eslint-plugin-markdown be of any help? We use it in CI and local linting.

Also, you can just run from the project root for linting code fragments in docs:

node tools/node_modules/eslint/bin/eslint.js --ext=.md doc

If this intended to refer brotli.constants property (and not just "Constants" section), then this should be #brotli_brotli_constants.

Do you think it would be possible to work on re-using as much code as possible? I think that would be a very good idea if we can make it work (because it’s going to happen eventually anyway).

I am unsure how this would be done for the C++ code, but it might be worth a shot for the JS.

I am not sure, but it seems a bit incomplete or confusing now. Maybe something like "and are simply ignored when inappropriate."?

Something like "and invalid options are simply ignored" should also work.

Refs: nodejs#20458 Co-authored-by: Hackzzila <admin@hackzzila.com>

Refs: #20458 Co-authored-by: Hackzzila <admin@hackzzila.com> PR-URL: #24938 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

This has been done in #24938! 🎉

Refs: nodejs#20458 Co-authored-by: Hackzzila <admin@hackzzila.com> PR-URL: nodejs#24938 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Refs: #20458 Co-authored-by: Hackzzila <admin@hackzzila.com> Backport-PR-URL: #27681 PR-URL: #24938 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Hackzzila added 4 commits April 30, 2018 23:24

deps: add brotli

a295f2a

brotli: add brotli

5d82880

benchmark,test: add brotli

52e2070

doc, test: add brotli and fix tests

2154080

vsemozhetbyt added the semver-minor PRs that contain new features and should be released in the next minor version. label May 1, 2018

vsemozhetbyt added the tsc-review label May 1, 2018

vsemozhetbyt reviewed May 1, 2018

View reviewed changes

vsemozhetbyt added the brotli label May 2, 2018

devsnek reviewed May 2, 2018

View reviewed changes

doc: improve brotli docs

54aa2e7

vsemozhetbyt reviewed May 2, 2018

View reviewed changes

doc: make acceptEncoding const

69cc504

vsemozhetbyt reviewed May 2, 2018

View reviewed changes

doc: small brotli doc fixes

5eb194f

vsemozhetbyt reviewed May 2, 2018

View reviewed changes

doc: small doc fixes

72c2d4e

addaleax self-assigned this Dec 7, 2018

addaleax mentioned this pull request Dec 10, 2018

zlib: add Brotli support #24938

Closed

4 tasks

georgyberdyshev mentioned this pull request Dec 14, 2018

Meteor Auto-compress interfering with Brotli enabled nginx front-end proxy meteor/meteor#10377

Closed

addaleax added a commit to addaleax/node that referenced this pull request Jan 3, 2019

zlib: add brotli support

193da61

Refs: nodejs#20458 Co-authored-by: Hackzzila <admin@hackzzila.com>

addaleax closed this Jan 5, 2019

Francois-Francois mentioned this pull request Aug 19, 2019

Brotli support removed nuxt-community/axios-module#276

Open

This comment was marked as spam.

Sign in to view

+              [`Accept-Encoding`]: https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.3
+              [`ArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer
+              [`brotli/encode.h`]: https://github.com/google/brotli/blob/v1.0.4/c/include/brotli/encode.h
+              [`brotli/decode.h`]: https://github.com/google/brotli/blob/v1.0.4/c/include/brotli/decode.h

               [`Accept-Encoding`]: https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.3
               [`ArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer
-              [`brotli/encode.h`]: https://github.com/google/brotli/blob/v1.0.4/c/include/brotli/encode.h
+              [`brotli.constants`]: #brotli_constants

-              Note that some options are only relevant when compressing, and are
-              ignored by the decompression classes.
+              Note that some options are only relevant when compressing or decompressing,
+              and are simply ignored.

		@@ -81,6 +81,10 @@ added: v9.6.0

		Enable experimental ES Module support in the `vm` module.

		### `--expose-brotli`


		## Threadpool Usage

		Note that all brotli APIs except those that are explicitly synchronous use

+              content-encoding mechanism defined by
+              [HTTP](https://tools.ietf.org/html/rfc7230#section-4.2).
+              The HTTP [`Accept-Encoding`][] header is used within an http request to identify

+                // Note: This is not a conformant accept-encoding parser.
+                // See https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.3
+                // Note: A quality of 4 is a good balace of speed and quality.

		Error codes for decompression operations.

		## brotli.constants

+              to supply options to the `brotli` classes and will call the supplied callback
+              with `callback(error, result)`.
+              Every method has a `*Sync` counterpart, which accept the same arguments, but

+              Every method has a `*Sync` counterpart, which accept the same arguments, but
+              without a callback.
+              ### brotli.compress(buffer[, options][, callback])

+              - `buffer` {Buffer|TypedArray|DataView|ArrayBuffer|string}
+              Decompress a chunk of data with [Decompress][].
+              If callback is omitted a Promise will be returned.


		- `buffer` {Buffer\|TypedArray\|DataView\|ArrayBuffer\|string}

		Decompress a chunk of data with [Decompress][].

-                if (!acceptEncoding) {
-                  acceptEncoding = '';
-                }
+                let acceptEncoding = request.headers['accept-encoding'] || '';

		@@ -0,0 +1,404 @@
		# Brotli

brotli: add brotli support by Hackzzila · Pull Request #20458 · nodejs/node · GitHub

Uh oh!

Uh oh!

brotli: add brotli support - #20458

brotli: add brotli support#20458Hackzzila wants to merge 11 commits intonodejs:masternodejs/node:masterfrom Hackzzila:brotli-devHackzzila/node:brotli-devCopy head branch name to clipboard

Conversation

Hackzzila commented May 1, 2018

Checklist

Uh oh!

addaleax commented May 1, 2018

Uh oh!

devsnek commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devsnek commented May 1, 2018

Uh oh!

Hackzzila commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devongovett commented May 1, 2018

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hackzzila May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hackzzila commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented May 2, 2018

Uh oh!

vsemozhetbyt commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hackzzila commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

brotli: add brotli support#20458
Hackzzila wants to merge 11 commits into
nodejs:masterfrom
Hackzzila:brotli-dev

devsnek commented May 1, 2018 •
edited
Loading

vsemozhetbyt commented May 1, 2018 •
edited
Loading

Hackzzila commented May 1, 2018 •
edited
Loading

Hackzzila May 1, 2018 •
edited
Loading

vsemozhetbyt May 1, 2018 •
edited
Loading

vsemozhetbyt May 2, 2018 •
edited
Loading