Skip to content
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 support for info: true option #2691

Merged
merged 1 commit into from
Sep 12, 2024
Merged

zlib: Add support for info: true option #2691

merged 1 commit into from
Sep 12, 2024

Conversation

npaun
Copy link
Member

@npaun npaun commented Sep 11, 2024

This PR implements the {info: true} option for zlib. It's not documented for brotli, but the unit tests expect it there too. We return an engine, and the buffer.

However, the engine we're returning isn't really legit - we just create it on the fly to return it. Should we fall back to the streaming path when info: true, for more fidelity with NodeJS or is this close enough? Now going whole-hog and falling back to the streaming implementation when info: true

@npaun npaun requested review from a team as code owners September 11, 2024 14:48
@npaun npaun linked an issue Sep 11, 2024 that may be closed by this pull request
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

We're diverging from node.js zlib implementation and it is producing different states for info: true.

For example, on Node.js

> require('zlib').brotliCompressSync('aaa', { info: true })
{
  buffer: <Buffer 0b 01 80 61 61 61 03>,
  engine: BrotliCompress {
    _writeState: Uint32Array(2) [ 16377, 0 ],

but calling initializing BrotliCompress directly produces different results on writeState

> require('zlib').BrotliCompress({ info: true })
<ref *1> BrotliCompress {
  _writeState: Uint32Array(2) [ 0, 0 ],

src/node/internal/internal_zlib.ts Outdated Show resolved Hide resolved
@@ -250,3 +248,13 @@ export class BrotliEncoder extends CompressionStream {
): boolean;
public params(): void;
}

export const enum ZlibMode {
Copy link
Member

@anonrig anonrig Sep 12, 2024

Choose a reason for hiding this comment

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

Enum values are not 1 to 1 representably js. We should avoid using them. Can we convert this to an object and move this to the file where it is used?

Copy link
Member Author

@npaun npaun Sep 12, 2024

Choose a reason for hiding this comment

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

I moved this enum to internal_zlib.ts, but I was not sure how to replace it with an object, because keyof typeof CLASS_BY_MODE would just be number.

src/node/internal/zlib.d.ts Outdated Show resolved Hide resolved
src/node/internal/internal_zlib.ts Outdated Show resolved Hide resolved
src/node/internal/internal_zlib.ts Outdated Show resolved Hide resolved
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Let's wait for @jasnell to review, before landing.

@npaun npaun merged commit 71ed2a6 into main Sep 12, 2024
13 checks passed
@npaun npaun deleted the npaun/info-true branch September 12, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing { info: true } support on node:zlib
3 participants