zlib: option for engine in convenience methods #13089
Conversation
There was a problem hiding this comment.
Hi, thanks for this, can you add documentation? In the absence of documentation its difficult to figure out what the code is inte 8000 nded to do, or to review the code to see if its behaving as intended.
4406738 to
2c36616
Compare
Added option to expose engine to convenience methods Refs: nodejs#8874
2c36616 to
e5ca118
Compare
|
@mscdex Why is this semver-major? |
|
@addaleax Because of the possible change in values passed to the callback or returned by the sync function? Previously it was always |
|
@mscdex Yes, but it’s explicitly opt-in. The returned value is an object only when |
|
@addaleax Probably. Docs should be added I think, especially in case someone unknowingly passes an object with |
|
@AlexanderOMara To be clear, once you add docs here this should be good. 👍 |
|
I've added some documentation. Let me know if it needs any changes. |
doc/api/zlib.md
Outdated
| * `strategy` {integer} (compression only) | ||
| * `dictionary` {Buffer|TypedArray|DataView} (deflate/inflate only, empty dictionary by | ||
| default) | ||
| * `info` {boolean} (It `true`, returns object with buffer and engine) |
There was a problem hiding this comment.
s/It/If/
s/returns object/returns an object/
Also, perhaps put 'buffer' and 'engine' in backticks to give a hint that they are the actual object property names.
There was a problem hiding this comment.
Good call! Edits made.
| * `strategy` {integer} (compression only) | ||
| * `dictionary` {Buffer|TypedArray|DataView} (deflate/inflate only, empty dictionary by | ||
| default) | ||
| * `info` {boolean} (If `true`, returns an object with `buffer` and `engine`) |
There was a problem hiding this comment.
nit: I’d drop the parentheses, and maybe add properties at the end to be explicit
|
Landed in d0b1b52 |
|
Release team decided not to backport to v6.x. |
Added option to expose engine to convenience methods
Refs: #8874
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
Alternative to Pull Request #12939, broken into 2 separate commits.