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

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-01T21:50:19Z

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)

doc/api/brotli.md

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


vsemozhetbyt · 2018-05-01T22:16:39Z

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-01T22:21:28Z

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-01T22:37:11Z

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?

doc/api/brotli.md

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


vsemozhetbyt · 2018-05-01T23:35:25Z

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:40:05Z

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:40:59Z

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:42:52Z

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?

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

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

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

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

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.

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.

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>

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

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

doc/api/brotli.md Outdated

Compress a stream.

### new brotli.Compress(options)

Copy link

Contributor

vsemozhetbyt May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[options]?

vsemozhetbyt reviewed May 1, 2018

View reviewed changes

doc/api/brotli.md Outdated

Decompress a stream.

### new brotli.Decompress(options)

Copy link

Contributor

vsemozhetbyt May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[options]?

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

rikless 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hackzzila May 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

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

Hackzzila commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt May 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented May 2, 2018

vsemozhetbyt commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hackzzila commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt May 2, 2018 • edited

Choose a reason for hiding this comment

Hackzzila commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jan 5, 2019

This comment was marked as spam.

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