GeezerVison

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

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.

@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

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.