zlib: support Uint8Array in convenience methods #12001

TimothyGu · 2017-03-23T03:43:43Z

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

Affected core subsystem(s)

zlib

addaleax · 2017-03-23T03:51:12Z

lib/zlib.js

@@ -78,6 +79,11 @@ function isInvalidStrategy(strategy) {
 }

 function zlibBuffer(engine, buffer, callback) {
+  // Streams do not support non-Buffer Uint8Arrays yet. Convert it to a
+  // Buffer without copying.
+  if (isUint8Array(buffer) && !Buffer.isBuffer(buffer))


You can use the plain Object.getPrototypeOf() check here, that should give better performance in the common case and won’t hurt

addaleax · 2017-03-23T03:53:02Z

lib/zlib.js

-  if (!(buffer instanceof Buffer))
-    throw new TypeError('Not a string or buffer');
+  else if (!isUint8Array(buffer))
+    throw new TypeError('Not a string, a Buffer, or an Uint8Array');


I guess it might depend on how you pronounce it, but isn’t it a Uint8Array?

As long as the error message is being changed, it would be better to make it more consistent ...

e.g.

"buffer" must be a string, Buffer, or Uint8Array

Yeah I was thinking of "unsigned 8-bit integer array" which would indeed be "an". @jasnell's suggestion looks better though.

TimothyGu · 2017-03-23T05:28:41Z

@addaleax, @jasnell comments addressed; PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/6993/

lpinca · 2017-03-24T07:32:58Z

doc/api/zlib.md

 -->

-Compress a [Buffer][] or string with [Deflate][].
+- `buffer` {Buffer|Uint8Array|string}


I think there should be a space before and after |.

The docs are inconsistent - sometimes spaces, sometimes not - but no spaces appears to be commonest.

lpinca · 2017-03-24T07:50:24Z

doc/api/zlib.md

 -->

-Decompress a [Buffer][] or string with [Unzip][].
+- `buf` {Buffer|Uint8Array|string}


s/buf/buffer/

lpinca · 2017-03-24T08:27:14Z

test/parallel/test-zlib-convenience-methods.js

-const expect = 'blahblahblahblahblahblah';
+const expectStr = 'blahblahblahblahblahblah';
+const expectBuf = Buffer.from(expectStr);
+const expectUint8Array = new Uint8Array([...expectBuf]);


Nit: I would pass the buffer directly in the Uint8Array constructor.

lpinca · 2017-03-24T08:49:12Z

test/parallel/test-zlib-dictionary.js

@@ -41,6 +41,7 @@ const spdyDict = Buffer.from([
  'ation/xhtmltext/plainpublicmax-agecharset=iso-8859-1utf-8gzipdeflateHTTP/1',
  '.1statusversionurl\0'
 ].join(''));
+const spdyDictUint8Array = new Uint8Array([...spdyDict]);


Nit: I would pass the buffer directly in the Uint8Array constructor.

addaleax · 2017-03-27T20:38:43Z

Ping @nodejs/ctc, this needs one more approval

bnoordhuis

Style nits but nothing major. The only thing that baffles me is the way the 'is buffer?' check is performed.

bnoordhuis · 2017-03-27T20:46:15Z

doc/api/zlib.md

-* `dictionary` (deflate/inflate only, empty dictionary by default)
+* `flush` {integer} (default: `zlib.constants.Z_NO_FLUSH`)
+* `finishFlush` {integer} (default: `zlib.constants.Z_FINISH`)
+* `chunkSize` {integer} (default: 16 × 1024)


We don't use × anywhere else in the documentation.

What do you suggest?

Just the 16*1024 that was there before.

bnoordhuis · 2017-03-27T20:47:59Z

doc/api/zlib.md

 -->

-Compress a [Buffer][] or string with [Deflate][].
+- `buffer` {Buffer|Uint8Array|string}


The docs are inconsistent - sometimes spaces, sometimes not - but no spaces appears to be commonest.

bnoordhuis · 2017-03-27T20:49:16Z

lib/zlib.js

+  if (isUint8Array(buffer) &&
+      !(Object.getPrototypeOf(buffer) === Buffer.prototype ||
+        buffer instanceof Buffer))
+    buffer = Buffer.from(buffer.buffer, buffer.byteOffset, buffer.byteLength);


Can you wrap the body in braces? Why the contorted Object.getPrototypeOf() and instanceof check?

@bnoordhuis I believe it’s because instanceof can be quite slow, so in the exceedingly common case the getPrototypeOf check suffices. But you’re right, I’d just drop the instanceof part.

bnoordhuis · 2017-03-27T20:54:09Z

test/parallel/test-zlib-not-string-or-buffer.js

@@ -7,7 +7,8 @@ require('../common');
 const assert = require('assert');
 const zlib = require('zlib');

-const expected = /^TypeError: Not a string or buffer$/;
+const expected =
+  /^TypeError: "buffer" argument must be a string, Buffer, or Uint8Array$/;


Teeny-tiny style nit: four space indent.

Also support Uint8Array as a `dictionary` option.

- doc: do not use Unicode multiplication sign - doc: fix incorrect parameter name - zlib: remove `instanceof Buffer` - zlib: stylistic change - test: remove unneeded array spread - test: stylistic change

TimothyGu · 2017-03-30T02:08:37Z

@lpinca @bnoordhuis Comments addressed. PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/7098/

TimothyGu · 2017-03-31T20:23:16Z

@bnoordhuis ping

outdated review

addaleax · 2017-04-03T07:55:11Z

Landed in 91383e4

Also support Uint8Array as a `dictionary` option. PR-URL: #12001 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](nodejs@4a7233c178)] [nodejs#12892](nodejs#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](nodejs@d2d32ea5a2)] [nodejs#11968](nodejs#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](nodejs@7eb1b4658e)] [nodejs#12141](nodejs#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](nodejs@beca3244e2)] [nodejs#10236](nodejs#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](nodejs@97a77288ce)] [nodejs#12348](nodejs#12348), [[`d75fdd96aa`](nodejs@d75fdd96aa)] [nodejs#10423](nodejs#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](nodejs@627ecee9ed)] [nodejs#10653](nodejs#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](nodejs@f18e08d820)] [nodejs#9744](nodejs#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](nodejs@3c3b36af0f)] [nodejs#12936](nodejs#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](nodejs@60d1aac8d2)] [nodejs#12784](nodejs#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](nodejs@84dabe8373)] [nodejs#12489](nodejs#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](nodejs@7a55e34ef4)] [nodejs#10467](nodejs#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](nodejs@3c2a9361ff)] [nodejs#9683](nodejs#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](nodejs@90403dd1d0)] [nodejs#11567](nodejs#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](nodejs@d3480776c7)] [nodejs#11259](nodejs#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](nodejs@fb71ba4921)] [nodejs#11355](nodejs#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](nodejs@3e6f1032a4)] [nodejs#10805](nodejs#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](nodejs@5de3cf099c)] [nodejs#10116](nodejs#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](nodejs@84a23391f6)] [nodejs#12113](nodejs#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](nodejs@56e881d0b0)] [nodejs#11975](nodejs#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](nodejs@03e89b3ff2)] [nodejs#10116](nodejs#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](nodejs@dd20e68b0f)] [nodejs#12725](nodejs#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](nodejs@3f27f02da0)] [nodejs#11599](nodejs#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (nodejs@ec7cbaf266)] [nodejs#12995](nodejs#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](nodejs@a16b570f8c)] [nodejs#11968](nodejs#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](nodejs@010f864426)] [nodejs#12949](nodejs#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](nodejs@a5f91ab230)] [nodejs#11689](nodejs#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](nodejs@8a7db9d4b5)] [nodejs#12087](nodejs#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](nodejs@b6e1d22fa6)] [nodejs#12925](nodejs#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](nodejs@07c7f198db)] [nodejs#12828](nodejs#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](nodejs@348cc80a3c)] [nodejs#5923](nodejs#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](nodejs@a2ae08999b)] [nodejs#11349](nodejs#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](nodejs@d523eb9c40)] [nodejs#11447](nodejs#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](nodejs@d080ead0f9)] [nodejs#12710](nodejs#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](nodejs@5bfd13b81e)] [nodejs#9726](nodejs#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](nodejs@455e6f1dd8)] [nodejs#11708](nodejs#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](nodejs@aab0d202f8)] [nodejs#11624](nodejs#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](nodejs@99da8e8e02)] [nodejs#12442](nodejs#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](nodejs@91383e47fd)] [nodejs#12001](nodejs#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](nodejs@b514bd231e)] [nodejs#11391](nodejs#11391).

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](4a7233c178)] [#12892](#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](d2d32ea5a2)] [#11968](#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](7eb1b4658e)] [#12141](#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](beca3244e2)] [#10236](#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](97a77288ce)] [#12348](#12348), [[`d75fdd96aa`](d75fdd96aa)] [#10423](#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](627ecee9ed)] [#10653](#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](f18e08d820)] [#9744](#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](3c3b36af0f)] [#12936](#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](60d1aac8d2)] [#12784](#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](84dabe8373)] [#12489](#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](7a55e34ef4)] [#10467](#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](3c2a9361ff)] [#9683](#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](90403dd1d0)] [#11567](#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](d3480776c7)] [#11259](#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](fb71ba4921)] [#11355](#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](3e6f1032a4)] [#10805](#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](5de3cf099c)] [#10116](#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](84a23391f6)] [#12113](#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](56e881d0b0)] [#11975](#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](03e89b3ff2)] [#10116](#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](dd20e68b0f)] [#12725](#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](3f27f02da0)] [#11599](#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (ec7cbaf266)] [#12995](#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](a16b570f8c)] [#11968](#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](010f864426)] [#12949](#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](a5f91ab230)] [#11689](#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](8a7db9d4b5)] [#12087](#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](b6e1d22fa6)] [#12925](#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](07c7f198db)] [#12828](#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](348cc80a3c)] [#5923](#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](a2ae08999b)] [#11349](#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](d523eb9c40)] [#11447](#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](d080ead0f9)] [#12710](#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](5bfd13b81e)] [#9726](#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](455e6f1dd8)] [#11708](#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](aab0d202f8)] [#11624](#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](99da8e8e02)] [#12442](#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](91383e47fd)] [#12001](#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](b514bd231e)] [#11391](#11391).

nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Mar 23, 2017

TimothyGu force-pushed the buffer-zlib branch 2 times, most recently from 16e9c83 to 73b3219 Compare March 23, 2017 03:50

addaleax approved these changes Mar 23, 2017

View reviewed changes

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 23, 2017

lpinca approved these changes Mar 24, 2017

View reviewed changes

targos mentioned this pull request Mar 27, 2017

zlib fails on Uint8Array #12072

Closed

jasnell approved these changes Mar 27, 2017

View reviewed changes

bnoordhuis previously requested changes Mar 27, 2017

View reviewed changes

TimothyGu added 2 commits March 29, 2017 19:01

zlib: support Uint8Array in convenience methods

874377b

Also support Uint8Array as a `dictionary` option.

Address review comments

0a41bf0

- doc: do not use Unicode multiplication sign - doc: fix incorrect parameter name - zlib: remove `instanceof Buffer` - zlib: stylistic change - test: remove unneeded array spread - test: stylistic change

TimothyGu force-pushed the buffer-zlib branch from 140336d to 0a41bf0 Compare March 30, 2017 02:02

addaleax closed this Apr 3, 2017

TimothyGu deleted the buffer-zlib branch April 3, 2017 08:17

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib: support Uint8Array in convenience methods #12001

zlib: support Uint8Array in convenience methods #12001

TimothyGu commented Mar 23, 2017

addaleax Mar 23, 2017

addaleax Mar 23, 2017

jasnell Mar 23, 2017

TimothyGu Mar 23, 2017

TimothyGu commented Mar 23, 2017

lpinca Mar 24, 2017

bnoordhuis Mar 27, 2017

lpinca Mar 24, 2017

lpinca Mar 24, 2017

lpinca Mar 24, 2017

addaleax commented Mar 27, 2017

bnoordhuis left a comment

bnoordhuis Mar 27, 2017

TimothyGu Mar 27, 2017

bnoordhuis Mar 28, 2017

bnoordhuis Mar 27, 2017

bnoordhuis Mar 27, 2017

addaleax Mar 27, 2017

bnoordhuis Mar 27, 2017

TimothyGu commented Mar 30, 2017 •

edited

Loading

TimothyGu commented Mar 31, 2017

addaleax commented Apr 3, 2017

zlib: support Uint8Array in convenience methods #12001

zlib: support Uint8Array in convenience methods #12001

Conversation

TimothyGu commented Mar 23, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Mar 27, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Mar 30, 2017 • edited Loading

TimothyGu commented Mar 31, 2017

addaleax commented Apr 3, 2017

TimothyGu commented Mar 30, 2017 •

edited

Loading