brotli: add brotli support #20458

Hackzzila · 2018-05-01T21:32:51Z

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


        deps: add brotli


        brotli: add brotli


        benchmark,test: add brotli


        doc, test: add brotli and fix tests

addaleax · 2018-05-01T21:35:29Z

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

devsnek · 2018-05-01T21:47:13Z

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)

vsemozhetbyt · 2018-05-01T21:56:49Z

As per Introducing New Modules policy, beware:

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

devsnek · 2018-05-01T22:01:23Z

cc @devongovett

Hackzzila · 2018-05-01T22:01:37Z

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

devongovett · 2018-05-01T22:09:27Z

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?

vsemozhetbyt

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

And thank you!

vsemozhetbyt · 2018-05-01T23:47:47Z

doc/api/cli.md

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

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

+### `--expose-brotli`
+


Should we add metadata?



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)

vsemozhetbyt · 2018-05-01T23:47:47Z

doc/api/brotli.md

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


Should we add  placeholder?

vsemozhetbyt · 2018-05-01T23:47:47Z

doc/api/brotli.md

+
+## Threadpool Usage
+
+Note that all brotli APIs except those that are explicitly synchronous use


brotli APIs -> `brotli` APIs?

vsemozhetbyt · 2018-05-01T23:47:47Z

doc/api/brotli.md

+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


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

vsemozhetbyt · 2018-05-01T23:47:47Z

doc/api/brotli.md

+
+  // 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.


balace ->balance?

vsemozhetbyt · 2018-05-01T23:47:47Z

doc/api/brotli.md

+Error codes for decompression operations.
+
+## brotli.constants
+


vsemozhetbyt · 2018-05-01T23:47:47Z

doc/api/brotli.md

+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


accept -> accepts?

vsemozhetbyt · 2018-05-01T23:47:47Z

doc/api/brotli.md

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


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

vsemozhetbyt · 2018-05-01T23:47:47Z

doc/api/brotli.md

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


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

vsemozhetbyt · 2018-05-01T23:47:47Z

doc/api/brotli.md

+
+- `buffer` {Buffer|TypedArray|DataView|ArrayBuffer|string}
+
+Decompress a chunk of data with [Decompress][].


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

Hackzzila · 2018-05-01T23:53:23Z

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

vsemozhetbyt · 2018-05-01T23:53:54Z

doc/api/brotli.md

+
+Compress a stream.
+
+### new brotli.Compress(options)


[options]?

vsemozhetbyt · 2018-05-01T23:55:07Z

doc/api/brotli.md

+
+Decompress a stream.
+
+### new brotli.Decompress(options)


[options]?

vsemozhetbyt · 2018-05-01T23:57:24Z

doc/api/brotli.md

+[`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


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 · 2018-05-02T00:04:23Z

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

vsemozhetbyt · 2018-05-02T00:08:42Z

I will go ahead and create brotli label)

devsnek · 2018-05-02T00:13:18Z

doc/api/brotli.md

+
+  // 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.


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


        doc: improve brotli docs

Hackzzila · 2018-05-02T01:27:28Z

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

vsemozhetbyt · 2018-05-02T01:35:19Z

doc/api/brotli.md

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


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


        doc: make acceptEncoding const

vsemozhetbyt · 2018-05-02T01:42:01Z

doc/api/brotli.md

 [`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


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

Hackzzila · 2018-05-02T01:44:43Z

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.


        doc: small brotli doc fixes

vsemozhetbyt · 2018-05-02T01:54:45Z

doc/api/brotli.md

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


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.


        doc: small doc fixes


        zlib: add brotli support

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


        zlib: add brotli support

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>


        zlib: add brotli support

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>

addaleax · 2019-01-05T21:20:29Z

This has been done in #24938! 🎉


        zlib: add brotli support

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>


        zlib: add brotli support

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>


        zlib: add brotli support

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>


        zlib: add brotli support

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>

mohammad-masud · 2019-08-23T05:58:55Z

very informative for my website called computer keyboards

Hackzzila added 4 commits May 1, 2018

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 label May 1, 2018

vsemozhetbyt added the tsc-review label May 1, 2018

vsemozhetbyt reviewed May 1, 2018

View changes

doc/api/brotli.md Outdated

Compress a stream.

### new brotli.Compress(options)

This comment has been minimized.

Sign in to view

Copy link

vsemozhetbyt May 1, 2018
Contributor

[options]?

vsemozhetbyt reviewed May 1, 2018

View changes

doc/api/brotli.md Outdated

Decompress a stream.

### new brotli.Decompress(options)

This comment has been minimized.

Sign in to view

Copy link

vsemozhetbyt May 1, 2018
Contributor

[options]?

vsemozhetbyt reviewed May 1, 2018

View changes

vsemozhetbyt added the brotli label May 2, 2018

devsnek reviewed May 2, 2018

View changes

doc: improve brotli docs

54aa2e7

vsemozhetbyt reviewed May 2, 2018

View changes

doc: make acceptEncoding const

69cc504

vsemozhetbyt reviewed May 2, 2018

View changes

doc: small brotli doc fixes

5eb194f

vsemozhetbyt reviewed May 2, 2018

View 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 of 4 tasks complete

georgyberdyshev mentioned this pull request Dec 14, 2018

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

Closed

addaleax closed this Jan 5, 2019

rikless mentioned this pull request Aug 19, 2019

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

Open

Jun	JUL	Aug
	05
2019	2020	2021

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

		Error codes for decompression operations.

		## brotli.constants


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

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

nodejs / node

Join GitHub today

brotli: add brotli support #20458

brotli: add brotli support #20458

Conversation

Hackzzila commented May 1, 2018

Checklist

addaleax commented May 1, 2018

devsnek commented May 1, 2018 • edited

vsemozhetbyt commented May 1, 2018 • edited

devsnek commented May 1, 2018

Hackzzila commented May 1, 2018 • edited

devongovett commented May 1, 2018

vsemozhetbyt left a comment

This comment has been minimized.

This comment has been minimized.

Hackzzila May 1, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Hackzzila commented May 1, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vsemozhetbyt May 1, 2018 • edited

This comment has been minimized.

This comment has been minimized.

vsemozhetbyt commented May 2, 2018

vsemozhetbyt commented May 2, 2018

This comment has been minimized.

This comment has been minimized.

Hackzzila commented May 2, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vsemozhetbyt May 2, 2018 • edited

Hackzzila commented May 2, 2018

This comment has been minimized.

This comment has been minimized.

addaleax commented Jan 5, 2019

mohammad-masud commented Aug 23, 2019

devsnek commented May 1, 2018 •

edited

vsemozhetbyt commented May 1, 2018 •

edited

Hackzzila commented May 1, 2018 •

edited

Hackzzila May 1, 2018 •

edited

vsemozhetbyt May 1, 2018 •

edited

vsemozhetbyt May 2, 2018 •

edited