zlib: add ArrayBuffer support by jemjam · Pull Request #16042 · nodejs/node · GitHub

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, src

We'll have to run this through CI before landing, but CI is currently busy with the 220+ code+learn PRs... so let's wait a bit.

Looks like this line might exceed 80 characters... either you can re-wrap, or we can do it while landing this.

These two lines can be joined into one. Again, it would be great if you could fix that, but if not we can fix that while landing.

This line needs to be wrapped at 80 characters as well.

LGTM

I know this is tricky, but can we actually split the non-zlib changes into its own commit? Can happen while landing as well :)

CI: https://ci.nodejs.org/job/node-test-commit/12848/

I addressed those couple of line length issues. Is there a code style guide somewhere that I should reference in the future? (I just ran my changes through eslint to get an idea of what to change.)

@addaleax did you link to some CI issues there? Looks like some failures. I had a glance but am not certain about the output there.

Also: when you suggested splitting were you talking the C++ code for the ArrayBuffers? Big thanks to @TimothyGu for holding my hand during my updating those; however I thought the work was pretty interrelated? If you want that part split, can it happen "during landing" as you suggested.

@JemBijoux I started CI jobs for this, yes. The failures look like this:

    assert.js:45
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 'The "buffer" argument must be one of type string, Buffer, TypedArray, DataView, or ArrayBuffer' === 'The "buffer" argument must be one of type string, Buffer, TypedArray, or DataView'
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1610-x64/test/common/index.js:723:16)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1610-x64/test/common/index.js:517:15)
        at expectedException (assert.js:651:19)
        at innerThrows (assert.js:685:21)
        at Function.throws (assert.js:702:3)
        at Object.expectsError (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1610-x64/test/common/index.js:745:12)
        at __dirname.forEach (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1610-x64/test/parallel/test-zlib-not-string-or-buffer.js:10:10)
        at Array.forEach (<anonymous>)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1610-x64/test/parallel/test-zlib-not-string-or-buffer.js:9:65)
        at Module._compile (module.js:600:30)

Can you reproduce those by running make test?

And, what I’m basically thinking is that the changes to src/node_buffer.cc could be in their own commit, because they are worth calling out explicitly in the changelog: They change the API for all native addon authors, and that’s not really obvious if the commit is referring only to zlib. That’s basically all. :)

Hey! So I was able to confirm the issue with the test and make that fix no problem. All tests passing again.

I also moved that node_buffer.cc file into it's own commit now (#16111). However, when I did that, one of the changes to a test I had here started to fail also in test/parallel/test-zlib-convenience-methods.js so I've removed that change for now.

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

I think this is ready to land. Ping @TimothyGu

@BridgeAR See also the discussion in #16111 … I am not quite at the point where I think extending the Buffer code for this is necessarily a great idea …

I think for now it might be nicer to let this be a zlib-only thing and create an Uint8Array for passed ArrayBuffers, then work with that, if we want support?

I see. In that case I just mark this accordingly for now. Thanks for the heads up!

I'm a little unsure what the situation is here. What would have to happen for this to get unblocked?

I'm a little unsure what the situation is here. What would have to happen for this to get unblocked?

Ping @BridgeAR @addaleax

ping @BridgeAR and @addaleax again to have another look ( one of 3 remaining code-and-learn PRs!)

This is blocked on #16111, that’s all.

I think for zlib this might make more sense than for the general case of all Buffer-taking APIs (although “compress a chunk of memory” is still going to be exceedingly rare compared to “compress a chunk of data”, conceptually).

It might be a nicer approach to create a Uint8Array over it when an ArrayBuffer is passed to the API? @TimothyGu ?

I've rebased this patch to be minimally invasive with regards to public APIs. @addaleax, @jasnell, @joyeecheung, @lpinca, @BridgeAR, @cjihrig, PTAL.

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

PR-URL: nodejs#16042 Refs: nodejs#1826 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Landed in 2848718 🎉

PR-URL: #16042 Refs: #1826 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 - Refactoring and cleanup of Http2Session and Http2Stream destroy (James M Snell) #17406 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

Would we want to backport to v8.x? If so this will need a manual backport

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. zlib Issues and PRs related to the zlib subsystem. labels Oct 6, 2017

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017

TimothyGu approved these changes Oct 7, 2017

View reviewed changes

cjihrig reviewed Oct 7, 2017

View reviewed changes

addaleax approved these changes Oct 7, 2017

View reviewed changes

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 7, 2017

jemjam mentioned this pull request Oct 9, 2017

Change node_buffer to accept ArrayBuffer #16111

Closed

3 tasks

lpinca approved these changes Oct 14, 2017

View reviewed changes

joyeecheung approved these changes Oct 14, 2017

View reviewed changes

TimothyGu self-assigned this Oct 14, 2017

jasnell approved these changes Oct 15, 2017

View reviewed changes

BridgeAR approved these changes Oct 18, 2017

View reviewed changes

BridgeAR added discuss Issues opened for discussions and feedbacks. blocked PRs that are blocked by other issues or PRs. labels Oct 18, 2017

Trott added code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. and removed code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. labels Oct 27, 2017

TimothyGu removed c++ Issues and PRs that require attention from people who are familiar with C++. discuss Issues opened for discussions and feedbacks. labels Dec 6, 2017

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

joyeecheung approved these changes Dec 11, 2017

View reviewed changes

jasnell approved these changes Dec 12, 2017

View reviewed changes

addaleax approved these changes Dec 13, 2017

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 13, 2017

addaleax closed this Dec 13, 2017

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 13, 2017

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added the backport-requested-v8.x label Aug 2, 2018

BeniCheni mentioned this pull request Oct 2, 2018

Accept ArrayBuffer (and typed array/data view?) anywhere Buffer is allowed in the API #1826

Closed

vsemozhetbyt mentioned this pull request Dec 21, 2018

doc: fix links in test/common/README.md #25172

Closed

3 tasks

zlib: add ArrayBuffer support by jemjam · Pull Request #16042 · nodejs/node · GitHub

Uh oh!

Uh oh!

zlib: add ArrayBuffer support - #16042

zlib: add ArrayBuffer support#16042jemjam wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jemjam:zlib-array-bufferCopy head branch name to clipboard

Conversation

jemjam commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 7, 2017

Uh oh!

jemjam commented Oct 7, 2017

Uh oh!

addaleax commented Oct 7, 2017

Uh oh!

jemjam commented Oct 9, 2017

Uh oh!

lpinca commented Oct 14, 2017

Uh oh!

BridgeAR commented Oct 18, 2017

Uh oh!

addaleax commented Oct 18, 2017

Uh oh!

BridgeAR commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Oct 27, 2017

Uh oh!

Trott commented Oct 29, 2017

Uh oh!

gireeshpunathil commented Oct 31, 2017

Uh oh!

addaleax commented Oct 31, 2017

Uh oh!

TimothyGu commented Dec 6, 2017

Uh oh!

addaleax commented Dec 13, 2017

Uh oh!

MylesBorins commented Aug 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

zlib: add ArrayBuffer support#16042
jemjam wants to merge 1 commit into
nodejs:masterfrom
jemjam:zlib-array-buffer

jemjam commented Oct 6, 2017 •
edited
Loading

BridgeAR commented Oct 18, 2017 •
edited
Loading