zlib: add Brotli support by addaleax · Pull Request #24938 · nodejs/node · GitHub

Add Brotli support to the zlib module; in particular:

zlib.createBrotliCompress(), zlib.brotliCompress(), zlib.brotliCompressSync()
zlib.createBrotliDecompress(), zlib.brotliDecompress(), zlib.brotliDecompressSync()

The APIs are identical to the zlib ones, except for the way that some of the more algorithm-specific options are passed to the stream constructor and a missing .params() function (because Brotli does not support that).

This PR is based on @Hackzzila’s #20458, although the internals are quite different, and re-uses more of the existing zlib infrastructure.

Some more visible differences to the main PR are:

This does currently not add any “experimental” state to the APIs. I would be okay with marking them as experimental in the documentation, but:
- I don’t see us doing significant changes to the API (because it’s so similar to zlib’s existing ones)
- I don’t realistically see us removing this, once we are initially indicating that we want to support this, as it has become part of the web platform (→ Can I Use shows 86 % browser support)
- I’m -1 on putting this behind a compile-time or run-time flag. I don’t think that makes sense, given how much this relies on the existing, well-tested zlib foundations.
This adds methods to the zlib module, rather than introducing a new module. I expect this to be somewhat controversial, given that this makes the naming of the module seem somewhat unfortunate.
- It makes sense from an internals perspective. It adds relatively little extra overhead to the zlib JS code (only 100 extra lines out of 900).
- It makes sense from an API perspective, because the existing streams and the new Brotli streams almost completely share their API.
- It is semver-minor, unlike introducing a new module.
- It is unfortunate that the module name that we use for compression is zlib, and that name has been unforunate since the module was first created, but ultimately, our users don’t care which library the streams are actually backed by, and so we shouldn’t either.

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 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1884/pipeline

CI: https://ci.nodejs.org/job/node-test-pull-request/19395/

I would much rather this live in a separate module or perhaps in a new namespace (which would still require a new 'module' anyway -- but would be better for organizational purposes) that can house all compression methods. I don't think stuffing this into zlib just so it's semver-minor is the right way to go about things. How the two work or are implemented internally doesn't really matter IMO.

I don't think stuffing this into zlib just so it's semver-minor is the right way to go about things.

As I said in the PR description, that’s not the only reason for this.

How the two work or are implemented internally doesn't really matter IMO.

It does matter that said fact makes the APIs analogous, imo. It also means that a bug or feature request for one part of the API likely applies to the other part of the API as well.

For those reviewing you can look at the below subset to avoid seeing the addition of the brotli dep itself

70bb2464c0ca18a35dfe6f2022d05bd8e67817f8...32e89319e19bbbf5cc5632b7da3e3a697bd62487

Interesting approach, and the API looks quite simple and usable to me.

This would be a great way to address the issue.

It does matter that said fact makes the APIs analogous, imo. It also means that a bug or feature request for one part of the API likely applies to the other part of the API as well.

That's an internal concern though, not something the end user should care about.

Anyway, I still believe we should be doing this more correct from the get-go by incorporating and using a new namespace instead.

@mscdex if we move to a new namespace are we imagining compression/zlib and compression/brotli within a node.js namespace? or simply brotli as a separate namespace from zlib?

I'm+1 on the approach suggested by @addaleax. I have to review the code more to sign off tho

@mscdex if we move to a new namespace are we imagining compression/zlib and compression/brotli within a node.js namespace? or simply brotli as a separate namespace from zlib?

I recognize 'compression', 'compress', and similar names are taken in the npm ecosystem, so if the package owners are not ok with node core using those names, I would be ok with something like 'compression/zlib' or similar if we cannot find some other feasible single phrase to use as a namespace. If we cannot come to agreement on that I would at the very least like to see brotli support as a separate module then (e.g. require('brotli')).

Would it make sense to avoid overlapping with existing npm packages by using special characters?

require('#brotli');

or use a different method maybe:

require.internal('brotli');
require.stdlib('brotli');

@MayhemYDG please take a look at #21551 where we are exploring introducing a Node.js namespace in which we can put future modules. This would solve the problem in a slightly more elegant way imho

@mscdex would you be open to landing this within the zlib namespace if we have a specific action plan for improving the API once namespaces land?

I'd like to propose we do the following

rename zlib to compression within the nodejs namespace (mechanics tbd in new core modules go under a namespace #21551)
discuss aliasing old zlib api in the namespace for compatibility reasons depending on consensus based around legacy modules
- if we alias perhaps do so with a deprecation warning and remove within 2 version?

make zlib and brotli as their own modules with compression e.g. compression.zlib, compression.brotli

That sounds like replicating the original mistake here – we shouldn’t be naming anything zlib or brotli in order to refer to libraries. Once we have the namespace issue resolved somehow, it would imo be better to just alias our current zlib to a nodejs-namespaced compression module; but then again, that seems a bit pointless if we also provide a nodejs-namespaced zlib module for compatibiliy (which I imagine we might want to do for all core modules)…?

@addaleax I've updated the above plan to rename zlib and add a deprecation warning to the compat alias.

@mscdex would you be open to landing this within the zlib namespace if we have a specific action plan for improving the API once namespaces land?

No, as that is something that would probably happen eventually anyway if namespaces do get added.

One approach moving forward could be:

Introduce a new node:compression module that is essentially the current zlib module
Land brotli into node:compression
Make zlib and alias for node:compression
Eventually, deprecate zlib.

@mscdex Instead of just saying that the reasons I provided for putting this feature in zlib in the current situation are invalid, could you give a reason not to do so?

@mscdex Instead of just saying that the reasons I provided for putting this feature in zlib in the current situation are invalid, could you give a reason not to do so?

I hinted at this initially, but I believe it's confusing/misleading and not the correct place for this new API from an organizational standpoint since brotli is not zlib-compatible nor is it a format supported by the standard zlib library. Is that what you were asking for?

@mscdex Okay – I’m not convinced by this, tbh… none of the other formats under zlib are mutually compatible; and like you said yourself, people shouldn’t care what the underlying libraries are, so we can as well act as if zlib provided this algorithm?

Again, I understand that it’s unfortunate that the module was named zlib, but I think it would be more confusing for users to group almost identical APIs with almost identical functionality in different modules…

none of the other formats under zlib are mutually compatible

The formats supported by node are all supported by the actual underlying zlib library, so it makes sense that they're available under the 'zlib' module. It also makes sense because a format like gzip utilizes zlib for its compression/decompression. brotli does not utilize zlib in any way.

but I think it would be more confusing for users to group almost identical APIs with almost identical functionality in different modules

I also prefer that they all live under one module/namespace. I just believe that 'zlib' should not be that module/namespace.

/cc @nodejs/collaborators for opinions on the naming & reviews

LGTM

I don't think most people who use the zlib module necessarily know which compression formats are supported by the "real" zlib or would ever run into issues because of the naming confusion. As such it seems weird to make users jump through additional hoops just because some people know precisely which algorithms are implemented by the C library of the same name.

Renaming it longer term to @nodejs/compress sounds like good cleanup but I'd be +1 to landing it in gzip as-is to make sure people can use it and find it easily.

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

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>

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>

Co-authored-by: Anna Henningsen <anna@addaleax.net> 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>

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>

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>

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>

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>

Co-authored-by: Anna Henningsen <anna@addaleax.net> 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>

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>

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>

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>

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>

Notable changes: - deps: - icu 63.1 bump (CLDR 34) (Steven R. Loomis) [#23715](#23715) - upgrade npm to 6.9.0 (Kat Marchán) [#26244](#26244) - upgrade openssl sources to 1.1.1a (Sam Roberts) [#25381](#25381) - upgrade to libuv 1.24.1 (cjihrig) [#25078](#25078) - events: add once method to use promises with EventEmitter (Matteo Collina) [#26078](#26078) - n-api: mark thread-safe function as stable (Gabriel Schulhof) [#25556](#25556) - repl: support top-level for-await-of (Shelley Vohr) [#23841](#23841) - zlib: - add brotli support (Anna Henningsen) [#24938](#24938) PR-URL: #27514

Notable changes: - **deps**: - update ICU to 64.2 (Ujjwal Sharma) [#27361](#27361) - upgrade npm to 6.9.0 (Kat Marchán) [#26244](#26244) - upgrade openssl sources to 1.1.1b (Sam Roberts) [#26327](#26327) - upgrade to libuv 1.28.0 (cjihrig) [#27241](#27241) - **events**: - add once method to use promises with EventEmitter (Matteo Collina) [#26078](#26078) - **n-api**: - mark thread-safe function as stable (Gabriel Schulhof) [#25556](#25556) - **repl**: - support top-level for-await-of (Shelley Vohr) [#23841](#23841) - **zlib**: - add brotli support (Anna Henningsen) [#24938](#24938) PR-URL: #27514

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Dec 10, 2018

addaleax mentioned this pull request Dec 10, 2018

zlib: split JS code as prep for non-zlib-backed streams #24939

Closed

2 tasks

mscdex suggested changes Dec 10, 2018

View reviewed changes

addaleax force-pushed the brotli branch from ae7dc7f to 32e8931 Compare December 10, 2018 20:45

mcollina approved these changes Dec 15, 2018

View reviewed changes

addaleax added the backported-to-v10.x label May 13, 2019

addaleax mentioned this pull request May 13, 2019

[v10.x] zlib: add brotli support #27681

Closed

4 tasks

bpierre mentioned this pull request May 15, 2019

Brotli support aragon/apm-serve#28

Open

ivan-aksamentov mentioned this pull request May 25, 2019

Consider switching to the BrotliCompress available in Node.js API nstepien/iltorb#92

Closed

totsteps mentioned this pull request Jul 3, 2019

Add Brotli support vercel/serve#543

Closed

aam1r mentioned this pull request Jun 12, 2020

Add Brotli support nodeca/pako#189

Closed

hybrist mentioned this pull request Mar 15, 2024

zlib: add zstd support #52100

Closed

zlib: add Brotli support by addaleax · Pull Request #24938 · nodejs/node · GitHub

Uh oh!

Uh oh!

zlib: add Brotli support - #24938

zlib: add Brotli support#24938addaleax wants to merge 8 commits intonodejs:masternodejs/node:masterfrom addaleax:brotliaddaleax/node:brotliCopy head branch name to clipboard

Conversation

addaleax commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Dec 10, 2018

Uh oh!

addaleax commented Dec 10, 2018

Uh oh!

mscdex left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Dec 10, 2018

Uh oh!

MylesBorins commented Dec 10, 2018

Uh oh!

kristoferbaxter commented Dec 10, 2018

Uh oh!

mscdex commented Dec 11, 2018

Uh oh!

MylesBorins commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Dec 11, 2018

Uh oh!

mscdex commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nstepien commented Dec 11, 2018

Uh oh!

MylesBorins commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Dec 11, 2018

Uh oh!

MylesBorins commented Dec 11, 2018

Uh oh!

mscdex commented Dec 11, 2018

Uh oh!

jasnell commented Dec 11, 2018

Uh oh!

addaleax commented Dec 11, 2018

Uh oh!

mscdex commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Dec 12, 2018

Uh oh!

mscdex commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Dec 15, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

hybrist commented Dec 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

19 participants

zlib: add Brotli support#24938
addaleax wants to merge 8 commits into
nodejs:masterfrom
addaleax:brotli

addaleax commented Dec 10, 2018 •
edited
Loading

MylesBorins commented Dec 11, 2018 •
edited
Loading

mscdex commented Dec 11, 2018 •
edited
Loading

MylesBorins commented Dec 11, 2018 •
edited
Loading

mscdex commented Dec 11, 2018 •
edited
Loading

mscdex commented Dec 13, 2018 •
edited
Loading