zlib: add Brotli support #24938

addaleax · Dec 10, 2018

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

nodejs-github-bot · Dec 10, 2018

@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

addaleax · Dec 10, 2018

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

mscdex · Dec 10, 2018

mscdex requested changes Dec 10, 2018

View changes

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.

addaleax · Dec 10, 2018

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.

MylesBorins · Dec 10, 2018

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

70bb246...32e8931

kristoferbaxter · Dec 10, 2018

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

This would be a great way to address the issue.

mscdex · Dec 11, 2018

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.

MylesBorins · Dec 11, 2018

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

jasnell · Dec 11, 2018

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

mscdex · Dec 11, 2018

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

MayhemYDG · Dec 11, 2018

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');

MylesBorins · Dec 11, 2018

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

addaleax · Dec 11, 2018

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)…?

MylesBorins · Dec 11, 2018

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

mscdex · Dec 11, 2018

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

jasnell · Dec 11, 2018

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.

addaleax · Dec 11, 2018

@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 · Dec 11, 2018

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

addaleax · Dec 12, 2018

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

mscdex · Dec 13, 2018

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.

addaleax · Dec 15, 2018

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

mcollina · Dec 15, 2018

mcollina approved these changes Dec 15, 2018

View changes

LGTM

jkrems · Dec 15, 2018

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.

MylesBorins · Jan 4, 2019

CI is green, I believe this is ready to land

addaleax · Jan 5, 2019

Landed in 345d3f3...b91093f 🎉

addaleax added zlib semver-minor notable-change brotli labels Dec 10, 2018

nodejs-github-bot added the meta label Dec 10, 2018

addaleax referenced this pull request Dec 10, 2018
Closed
zlib: split JS code as prep for non-zlib-backed streams #24939
2 of 2 tasks complete

addaleax force-pushed the addaleax:brotli branch from ae7dc7f to 32e8931 Dec 10, 2018

addaleax removed the tsc-agenda label Jan 5, 2019

addaleax closed this Jan 5, 2019

addaleax deleted the addaleax:brotli branch Jan 5, 2019

addaleax referenced this pull request Jan 5, 2019
Closed
brotli: add brotli support #20458
4 of 4 tasks complete

molant referenced this pull request Jan 6, 2019
Open
[Feature] Migrate brotli to built-in implementation #1604

ChALkeR referenced this pull request Jan 6, 2019
Closed
Feature Idea: Brotli support in core #18964

kanongil referenced this pull request Jan 7, 2019
Open
Support node brotli #4

voxpelli referenced this pull request Jan 7, 2019
Open
Support for Brotli compression #71

flyweight referenced this pull request Jan 16, 2019
Open
Support Brotli decompression #2866

BridgeAR referenced this pull request Jan 16, 2019
Merged
v11.7.0 proposal #25537

nodejs/node

Join GitHub today

zlib: add Brotli support #24938

Conversation

addaleax commented Dec 10, 2018 • edited

Checklist

addaleax added zlib semver-minor notable-change brotli labels Dec 10, 2018

This comment has been minimized.

nodejs-github-bot commented Dec 10, 2018

nodejs-github-bot added the meta label Dec 10, 2018

This comment has been minimized.

addaleax commented Dec 10, 2018

addaleax referenced this pull request Dec 10, 2018

mscdex requested changes Dec 10, 2018 View changes

mscdex left a comment

This comment has been minimized.

addaleax commented Dec 10, 2018

addaleax force-pushed the addaleax:brotli branch from ae7dc7f to 32e8931 Dec 10, 2018

This comment has been minimized.

MylesBorins commented Dec 10, 2018

This comment has been minimized.

kristoferbaxter commented Dec 10, 2018

This comment has been minimized.

mscdex commented Dec 11, 2018

This comment has been minimized.

MylesBorins commented Dec 11, 2018 • edited

This comment has been minimized.

jasnell commented Dec 11, 2018

This comment has been minimized.

mscdex commented Dec 11, 2018 • edited

This comment has been minimized.

MayhemYDG commented Dec 11, 2018

This comment has been minimized.

MylesBorins commented Dec 11, 2018 • edited

This comment has been minimized.

addaleax commented Dec 11, 2018

This comment has been minimized.

MylesBorins commented Dec 11, 2018

This comment has been minimized.

mscdex commented Dec 11, 2018

This comment has been minimized.

jasnell commented Dec 11, 2018

This comment has been minimized.

addaleax commented Dec 11, 2018

This comment has been minimized.

mscdex commented Dec 11, 2018 • edited

This comment has been minimized.

addaleax commented Dec 12, 2018

This comment has been minimized.

mscdex commented Dec 13, 2018 • edited

This comment has been minimized.

addaleax commented Dec 15, 2018

mcollina approved these changes Dec 15, 2018 View changes

mcollina left a comment

This comment has been minimized.

jkrems commented Dec 15, 2018

This comment has been minimized.

MylesBorins commented Jan 4, 2019

addaleax removed the tsc-agenda label Jan 5, 2019

This comment has been minimized.

addaleax commented Jan 5, 2019

addaleax closed this Jan 5, 2019

addaleax deleted the addaleax:brotli branch Jan 5, 2019

addaleax added a commit that referenced this pull request Jan 5, 2019

addaleax added a commit that referenced this pull request Jan 5, 2019

addaleax added a commit that referenced this pull request Jan 5, 2019

addaleax added a commit that referenced this pull request Jan 5, 2019

addaleax added a commit that referenced this pull request Jan 5, 2019

addaleax added a commit that referenced this pull request Jan 5, 2019

addaleax added a commit that referenced this pull request Jan 5, 2019

addaleax added a commit that referenced this pull request Jan 5, 2019

addaleax added a commit that referenced this pull request Jan 5, 2019

addaleax added a commit that referenced this pull request Jan 5, 2019

addaleax referenced this pull request Jan 5, 2019

molant referenced this pull request Jan 6, 2019

ChALkeR referenced this pull request Jan 6, 2019

kanongil referenced this pull request Jan 7, 2019

voxpelli referenced this pull request Jan 7, 2019

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

addaleax commented Dec 10, 2018 •

edited

mscdex requested changes Dec 10, 2018

View changes

addaleax force-pushed the addaleax:brotli branch from `ae7dc7f` to `32e8931` Dec 10, 2018

MylesBorins commented Dec 11, 2018 •

edited

mscdex commented Dec 11, 2018 •

edited

MylesBorins commented Dec 11, 2018 •

edited

mscdex commented Dec 11, 2018 •

edited

mscdex commented Dec 13, 2018 •

edited

mcollina approved these changes Dec 15, 2018

View changes