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

Regression in node:zlib polyfill in Deno v1.32.3 #19540

Closed
danopia opened this issue Jun 16, 2023 · 2 comments · Fixed by #20035
Closed

Regression in node:zlib polyfill in Deno v1.32.3 #19540

danopia opened this issue Jun 16, 2023 · 2 comments · Fixed by #20035
Assignees
Labels
bug Something isn't working correctly node compat

Comments

@danopia
Copy link
Contributor

danopia commented Jun 16, 2023

  • The dictionary parameter is not accepted anymore when setting up zlib.
  • I saw the regression with both createDeflate and createInflate.
  • Last working in Deno v1.32.2 and is still failing on the latest release.
  • Perhaps introduced by feat: port node:zlib to rust #18291
  • My usecase is implementing SPDY which has a predefined zlib dictionary for header compression.

Minimal reproduction, with zero-length dictionary:

import { Buffer } from 'node:buffer';
import { createDeflate } from 'node:zlib';

createDeflate({
  dictionary: Buffer.alloc(0),
});
console.log('All good!');
# nodejs runs the script fine:
$ node repro.js
All good!

# deno v1.34.2 throws:
$ deno run repro.js
error: Uncaught TypeError: Error parsing args at position 5: invalid type: byte array, expected a borrowed byte array
createInflate({
^
    at Zlib.init (ext:deno_node/_zlib_binding.mjs:146:21)
    at Inflate.Zlib (ext:deno_node/_zlib.mjs:352:16)
    at new Inflate (ext:deno_node/_zlib.mjs:214:8)
    at createInflate (ext:deno_node/_zlib.mjs:45:10)
    at file:///Users/daniellamando/Code/tmp.Rn9UISKm/repro.js:4:1

# last working deno version via a docker container:
$ podman run --rm -v (pwd):/src denoland/deno:alpine-1.32.2 run /src/repro.js
All good!

Also I noticed that Deno only allows Buffer instances for dictionary. As per nodejs docs, Uint8Array is accepted since node v8.0 and ArrayBuffer since node v9.4. So perhaps the polyfill should accept them too.

@danopia
Copy link
Contributor Author

danopia commented Aug 14, 2023

I just verified with the latest canary and zlib dictionaries are working again. Thanks a lot! <3

danopia added a commit to cloudydeno/deno-spdy_transport that referenced this issue Aug 14, 2023
This reverts commit e1885e0.

Deno merged a fix:
* denoland/deno#19540 (comment)

Should land in v1.36.2.
@bartlomieju
Copy link
Member

I just verified with the latest canary and zlib dictionaries are working again. Thanks a lot! <3

No problem, and sorry for breaking it 😓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly node compat
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants