node:zlib module has numerous bugs, inconsistencies, and errors

[nberlette]

I've noticed quite a few issues with the node:zlib implementation. Wasn't sure if I should open them up in individual issues, but decided to just do it unified since they're all related to the same module.

First off, I'd like to reference the official Node.js documentation on their zlib module. Note that the accepted input types by all convenience methods is Buffer | TypedArray | DataView | ArrayBuffer | string.

1. brotli compression fails on data > 4096B (fixed in fix(ext/node/ops/zlib/brotli): Allow decompressing more than 4096 bytes #20301)

2. zlib.brotliCompressSync only accepts inputs that are Buffer, TypedArray, or string. It accepts all types of TypedArray, it seems, but DataViews and ArrayBuffers cause it to throw a vague Failed to compress exception. Inconsistent with the expected behavior above. See below for a screenshot.

3. zlib.brotliCompress returns what appears to be a native Uint8Array, rather than a Node.js Buffer like its synchronous counterpart. Definitely inconsistent behavior, and it gets more inconsistent when trying different input types. Any type other than a Buffer, string, or Uint8Array throws an error saying it "was not a typed ArrayBufferView".... which is not true. See below.

4. The non-brotli convenience functions all seem to throw their own errors that have no discernable pattern or logic when it comes to which binary types should be allowed and which should be banned. Here's another screenshot showing gzip and gzipSync's erratic behavior:

I'd spend more time and document the behavior on each of the remaining methods but unfortunately I'm out of time right now. I'll try to follow this up with additional info and (hopefully) a PR, if I'm able to squeeze in a fix for this tonight/tomorrow.

Thanks!

[lucacasonato]

It looks like all is needed here is some massaging of input types into the correct forms for the internal algorithms, and the right output types for the reverse.

Sounds like a good first issue. @nberlette if you want to grab it, feel free, otherwise someone else can jump in (just remember to update on this issue if you are working on it as to not duplicate work!)

[dsherret]

Probably related: #20456

[nberlette]

@lucacasonato Thanks for the reply Luca, I'll take a stab at it tonight! Based on your response it sounds like it should be pretty straihtforward to sort out. But forgive me in advance for any late-night pings you may get if I find myself lost in internal Deno-API-land 😅.

@dsherret Thanks for bringing that issue in! Definitely seems to be related, thats some of the same behavior I've taken note of myself. I could swear I opened that issue up when I was searching for priors before making this post... I was in a rush though so I must've overlooked it. My bad.

I'll cc you guys in the PR when I get it pushed through. Have a great weekend!

[nberlette]

Hey @lucacasonato, hope all is well. I've been progressing on a PR for the zlib module, but other unexpected commitments took priority the last couple weeks. Apologies for the delays.

As this is my first real contribution here (aside from a few very small fixes in the past), I have a couple questions on aligning my changes with the codebase. Could you spare a few minutes to chat outside of GitHub, maybe via Discord or email?

Thanks for your patience,
Nick