zlib: refactor zlib module by jasnell · Pull Request #11391 · nodejs/node

jasnell · 2017-02-15T04:16:52Z

This does several significant things:

Refactors the structure of the zlib module, eliminating duplicated code, moving to ES6 classes for simplify inheritance, eliminating some closures and this->self uses. This was done in a way that should preserve API compat (no test changes were required at all for this piece).
Makes errors more consistent and improves testing of errors.

Perf impact should be minimal if any.

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

Trott · 2017-02-15T05:47:07Z

To be consistent with the other 3 arrow functions in the file and with the typical practice in the rest of our code base, the braces and explicit return should be dropped:
value: (instance)=>instanceinstanceoftype

targos

A few comments, but LGTM.

targos · 2017-02-15T11:12:03Z

Could this be moved to the internal util module? I think you had to do the same thing in the crypto module refactor and we could need this function in other places too.

Yep, it certainly could be.

targos · 2017-02-15T11:20:23Z

can you write the values like for flush flags?

targos · 2017-02-15T11:23:58Z

maybe keep the comment just above the class definition? feels wrong to see it here.

heh, totally overlooked the comment. will move

addaleax · 2017-02-15T14:58:13Z

Is the Type: tag intentionally missing?

nope, just missed it :-)

addaleax · 2017-02-15T15:10:40Z

These could be template strings, while you’re at it ;)

I'm going to be opening a PR that replaces these with the new internal/errors so wanted to leave these alone for now

addaleax · 2017-02-15T15:12:00Z

(ditto for template strings, likewise for the validators below)

addaleax · 2017-02-15T15:13:37Z

This would make a wonderful get _closed() { return !this._handle; } shorthand if you’re turning it into a class anyway :)

addaleax · 2017-02-15T15:16:54Z

Instead of defining Symbol.hasInstance, it might be better to set the actual fn.prototype to type.prototype

addaleax · 2017-02-15T15:17:24Z

Yes, I’m -1 on a runtime deprecation.

Ok, let's get more input from @nodejs/ctc on it. I have no problem pulling this back out for now, I'd just like for us to move away from this at some point.

jasnell · 2017-02-15T16:18:39Z

@targos @addaleax ... updated based on the feedback!

This still includes the proposed runtime deprecation. I have no issue dropping that from this PR but I'd like to get a sense of where @nodejs/ctc is on it in general first.

addaleax · 2017-02-15T16:22:46Z

If you’re using Reflect.construct anyway, you can actually be a bit more generic and say Reflect.construct(type, args, new.target || type)

Yeah... was just considering that...

Done! Added a test case for the new internal function also.

jasnell · 2017-02-16T00:00:04Z

@addaleax ... I've marked this as in progress because I'm working through one issue with the wrapper function approach... Specifically, it breaks old-style inheritance using util.inherits()... e.g.

classA{constructor(z){this.z=z;}}constB=internalUtil.createClassWrapper(A);// This worksclassCextendsB{constructor(z){super(z);}}// This doesn'tfunctionD(z){if(!(thisinstanceofD))returnnewD(z);B.call(this,z);}util.inherits(D,B);constc=newC(1);assert(cinstanceofA);// trueassert(cinstanceofB);// trueassert.strictEqual(c.z,1)// trueconstd=newD(1);assert(cinstanceofA);// falseassert(cinstanceofB);// falseassert.strictEqual(c.z,1)// false, z is not set

In the off chance that someone has extended one of the API classes, this will break their stuff so we need to be cautious.

jasnell · 2017-02-16T01:55:16Z

Searching on github and google show no hits for any pattern like Deflate.call(, Inflate.call(, etc that would indicate code that is inheriting from the classes using the old style. That doesn't mean such code doesn't exist but I believe the impact here will be minimal

addaleax · 2017-02-16T14:07:30Z

@jasnell I’m pretty sure subclassing the zlib classes isn’t something that really makes sense… this should be fine as a semver-major change.

sam-github

backporting is PR based, not commit based, can you break the PR into the backportable part (refactor) and the non-backportable semver-major?

changes LGTM

jasnell · 2017-02-16T14:46:33Z

I'll open a separate backport pr that pulls in the pieces that can be

Utility function for wrapping an ES6 class with a constructor function that does not require the new keyword

jasnell · 2017-02-16T16:31:55Z

@addaleax ... I dropped the deprecation of the old accessors from this. I'll open a separate PR that proposes that so it can be looked at independently.

jasnell · 2017-02-16T16:34:38Z

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

jasnell · 2017-02-16T19:35:37Z

@sam-github ... are you good with this now?

jasnell · 2017-02-16T20:49:27Z

CI failures are unrelated. If there are no objections I will land this on friday

jasnell · 2017-02-17T17:28:19Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/584/

Utility function for wrapping an ES6 class with a constructor function that does not require the new keyword PR-URL: #11391 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

PR-URL: #11391 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

jasnell · 2017-02-17T18:41:06Z

Landed in 4151ab3...b514bd2

* **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).

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 15, 2017

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

Trott reviewed Feb 15, 2017
View reviewed changes

targos approved these changes Feb 15, 2017
View reviewed changes

addaleax requested changes Feb 15, 2017
View reviewed changes

jasnell force-pushed the zlib-refactor branch 2 times, most recently from a83647d to c2071ddCompare February 15, 2017 16:17

addaleax reviewed Feb 15, 2017
View reviewed changes

jasnell force-pushed the zlib-refactor branch from c2071dd to cb91174Compare February 15, 2017 16:56

jasnell added the wip Issues and PRs that are still a work in progress. label Feb 15, 2017

sam-github suggested changes Feb 16, 2017
View reviewed changes

jasnell added 3 commits February 16, 2017 08:29

util: add createClassWrapper to internal/util
fd70000
Utility function for wrapping an ES6 class with a constructor function that does not require the new keyword

zlib: refactor zlib module
8badc1c

zlib: use RangeError/TypeError consistently
dacb5e6

jasnell force-pushed the zlib-refactor branch from cb91174 to dacb5e6Compare February 16, 2017 16:30

jasnell changed the title ~~zlib: refactor zlib module, deprecate old constant accessors~~zlib: refactor zlib moduleFeb 16, 2017

addaleax approved these changes Feb 16, 2017
View reviewed changes

jasnell removed the wip Issues and PRs that are still a work in progress. label Feb 16, 2017

sam-github approved these changes Feb 16, 2017
View reviewed changes

jasnell added a commit that referenced this pull request Feb 17, 2017
zlib: refactor zlib module
8e69f7e
PR-URL: #11391 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

jasnell added a commit that referenced this pull request Feb 17, 2017
zlib: use RangeError/TypeError consistently
b514bd2
PR-URL: #11391 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

jasnell closed this Feb 17, 2017

jasnell mentioned this pull request Apr 4, 2017
8.0.0 Release Proposal #12220
Closed

Uh oh!

Uh oh!

Conversation

jasnell commented Feb 15, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

addaleax commented Feb 16, 2017

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

jasnell commented Feb 17, 2017

Uh oh!

jasnell commented Feb 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jasnell commented Feb 15, 2017•
edited
Loading