zlib: add Brotli support by addaleax · Pull Request #24938 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation62 Commits8 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

jdalton, tomscholz, kristoferbaxter, bricss, styfle, tuananh, ginkoid, lucleray, lmammino, yisibl, and 25 more reacted with thumbs up emoji bnb, benjamingr, a0viedo, MDSLKTR, bricss, styfle, tuananh, ginkoid, dbrockman, lmammino, and 14 more reacted with hooray emoji bnb, benjamingr, cb1kenobi, MDSLKTR, nstepien, tomscholz, sindresorhus, bricss, styfle, ginkoid, and 12 more reacted with heart emoji

addaleax added zlib

Issues and PRs related to the zlib subsystem.

semver-minor

PRs that contain new features and should be released in the next minor version.

notable-change

PRs with changes that should be highlighted in changelogs.

brotli

Issues and PRs related to the brotli dependency.

labels

Dec 10, 2018

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

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.

addaleax pushed a commit to addaleax/node that referenced this pull request

May 13, 2019

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

addaleax added a commit to addaleax/node that referenced this pull request

May 13, 2019

Refs: nodejs#20458

Co-authored-by: Hackzzila admin@hackzzila.com

addaleax added a commit to addaleax/node that referenced this pull request

May 13, 2019

Co-authored-by: Hackzzila admin@hackzzila.com

addaleax added a commit to addaleax/node that referenced this pull request

May 13, 2019