New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zlib: add Brotli support #24938

Closed
wants to merge 8 commits into
base: master
from

Conversation

@addaleax
Copy link
Member

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

This comment has been minimized.

@addaleax 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
@mscdex
Copy link
Contributor

mscdex left a comment

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

This comment has been minimized.

Copy link
Member

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

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

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented 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

This comment has been minimized.

Copy link

kristoferbaxter commented 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

This comment has been minimized.

Copy link
Contributor

mscdex commented 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

This comment has been minimized.

Copy link
Member

MylesBorins commented 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

This comment has been minimized.

Copy link
Member

jasnell commented Dec 11, 2018

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

@mscdex

This comment has been minimized.

Copy link
Contributor

mscdex commented 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

This comment has been minimized.

Copy link
Contributor

MayhemYDG commented 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

This comment has been minimized.

Copy link
Member

MylesBorins commented 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

This comment has been minimized.

Copy link
Member

addaleax commented 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

This comment has been minimized.

Copy link
Member

MylesBorins commented Dec 11, 2018

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

@mscdex

This comment has been minimized.

Copy link
Contributor

mscdex commented 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

This comment has been minimized.

Copy link
Member

jasnell commented Dec 11, 2018

One approach moving forward could be:

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

This comment has been minimized.

Copy link
Member

addaleax commented 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

This comment has been minimized.

Copy link
Contributor

mscdex commented 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

This comment has been minimized.

Copy link
Member

addaleax commented 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

This comment has been minimized.

Copy link
Contributor

mscdex commented 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

This comment has been minimized.

Copy link
Member

addaleax commented Dec 15, 2018

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

@mcollina
Copy link
Member

mcollina left a comment

LGTM

@jkrems

This comment has been minimized.

Copy link
Contributor

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

@addaleax addaleax removed the tsc-agenda label Jan 5, 2019

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 5, 2019

Landed in 345d3f3...b91093f 🎉

@addaleax addaleax closed this Jan 5, 2019

@addaleax addaleax deleted the addaleax:brotli branch Jan 5, 2019

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

deps: add brotli
Co-authored-by: Anna Henningsen <anna@addaleax.net>

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 added a commit that referenced this pull request Jan 5, 2019

deps,tools: update license-builder.sh and LICENSE
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 added a commit that referenced this pull request Jan 5, 2019

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 added a commit that referenced this pull request Jan 5, 2019

benchmark,test: add brotli
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 added a commit that referenced this pull request Jan 5, 2019

doc: add documentation for brotli support
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 added a commit that referenced this pull request Jan 5, 2019

deps: add brotli
Co-authored-by: Anna Henningsen <anna@addaleax.net>

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 added a commit that referenced this pull request Jan 5, 2019

deps,tools: update license-builder.sh and LICENSE
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 added a commit that referenced this pull request Jan 5, 2019

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 added a commit that referenced this pull request Jan 5, 2019

benchmark,test: add brotli
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 added a commit that referenced this pull request Jan 5, 2019

doc: add documentation for brotli support
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 addaleax referenced this pull request Jan 5, 2019

Closed

brotli: add brotli support #20458

4 of 4 tasks complete

@kanongil kanongil referenced this pull request Jan 7, 2019

Open

Support node brotli #4

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

deps: add brotli
Co-authored-by: Anna Henningsen <anna@addaleax.net>

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>

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

deps,tools: update license-builder.sh and LICENSE
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>

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

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>

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

benchmark,test: add brotli
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>

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

doc: add documentation for brotli support
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>

@BridgeAR BridgeAR referenced this pull request Jan 16, 2019

Merged

v11.7.0 proposal #25537

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    nodejs#24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    nodejs#24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    nodejs#25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    nodejs#25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    nodejs#22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    nodejs#25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    nodejs#25006

PR-URL: nodejs#25537

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    nodejs#24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    nodejs#24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    nodejs#25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    nodejs#25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    nodejs#22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    nodejs#25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    nodejs#25006
* worker:
  * Expose workers by default and remove `--experimental-worker` flag
    (Anna Henningsen) nodejs#25361

PR-URL: nodejs#25537

BridgeAR added a commit that referenced this pull request Jan 18, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    #24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    #24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    #25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    #25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    #22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    #25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    #25006
* worker:
  * Expose workers by default and remove `--experimental-worker` flag
    (Anna Henningsen) #25361

PR-URL: #25537
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment