From 026c95a69578a7e1e4ed80ccf2680a036b8941ae Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 28 Sep 2016 19:47:20 +0200 Subject: [PATCH 1/5] stream: proper `instanceof` for `Writable`s Use `[Symbol.hasInstance]()` to return `true` when asking for `new Duplex() instanceof Writable`. --- doc/api/stream.md | 4 +++- lib/_stream_writable.js | 23 ++++++++++++++++--- test/parallel/test-stream-inheritance.js | 29 ++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-stream-inheritance.js diff --git a/doc/api/stream.md b/doc/api/stream.md index a6f35dc9c0282a..f1db0f93a1e981 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1559,7 +1559,8 @@ Because JavaScript does not have support for multiple inheritance, the to extending the `stream.Readable` *and* `stream.Writable` classes). *Note*: The `stream.Duplex` class prototypically inherits from `stream.Readable` -and parasitically from `stream.Writable`. +and parasitically from `stream.Writable`, but `instanceof` will work properly +for both base classes by overriding [`Symbol.hasInstance`][] Custom Duplex streams *must* call the `new stream.Duplex([options])` constructor and implement *both* the `readable._read()` and @@ -2009,3 +2010,4 @@ readable buffer so there is nothing for a user to consume. [Transform]: #stream_class_stream_transform [Writable]: #stream_class_stream_writable [zlib]: zlib.html +[`Symbol.hasInstance`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/hasInstance diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index 88c19cddbfa490..184e52f9b6935a 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -134,11 +134,28 @@ Object.defineProperty(WritableState.prototype, 'buffer', { 'instead.') }); +// Test _writableState for inheritance to account for Duplex streams, +// whose prototype chain only points to Readable. +var realHasInstance; +if (Symbol.hasInstance) { + realHasInstance = Function.prototype[Symbol.hasInstance]; + Object.defineProperty(Writable, Symbol.hasInstance, { + value: function(object) { + return object._writableState instanceof WritableState; + } + }); +} else { + realHasInstance = function(object) { + return object instanceof this; + }; +} + function Writable(options) { - // Writable ctor is applied to Duplexes, though they're not - // instanceof Writable, they're instanceof Readable. - if (!(this instanceof Writable) && !(this instanceof Stream.Duplex)) + // Writable ctor is applied to Duplexes, too. + if (!(realHasInstance.call(Writable, this)) && + !(this instanceof Stream.Duplex)) { return new Writable(options); + } this._writableState = new WritableState(options, this); diff --git a/test/parallel/test-stream-inheritance.js b/test/parallel/test-stream-inheritance.js new file mode 100644 index 00000000000000..efc28731bd0c36 --- /dev/null +++ b/test/parallel/test-stream-inheritance.js @@ -0,0 +1,29 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const { Readable, Writable, Duplex, Transform } = require('stream'); + +const readable = new Readable({ read() {} }); +const writable = new Writable({ write() {} }); +const duplex = new Duplex({ read() {}, write() {} }); +const transform = new Transform({ transform() {} }); + +assert.ok(readable instanceof Readable); +assert.ok(!(writable instanceof Readable)); +assert.ok(duplex instanceof Readable); +assert.ok(transform instanceof Readable); + +assert.ok(!(readable instanceof Writable)); +assert.ok(writable instanceof Writable); +assert.ok(duplex instanceof Writable); +assert.ok(transform instanceof Writable); + +assert.ok(!(readable instanceof Duplex)); +assert.ok(!(writable instanceof Duplex)); +assert.ok(duplex instanceof Duplex); +assert.ok(transform instanceof Duplex); + +assert.ok(!(readable instanceof Transform)); +assert.ok(!(writable instanceof Transform)); +assert.ok(!(duplex instanceof Transform)); +assert.ok(transform instanceof Transform); From a21153d3c34bd901a44a7dc7aeb193daecb65199 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 28 Sep 2016 19:56:11 +0200 Subject: [PATCH 2/5] [squash] fix typo --- doc/api/stream.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index f1db0f93a1e981..996d0db8346d34 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1560,7 +1560,7 @@ to extending the `stream.Readable` *and* `stream.Writable` classes). *Note*: The `stream.Duplex` class prototypically inherits from `stream.Readable` and parasitically from `stream.Writable`, but `instanceof` will work properly -for both base classes by overriding [`Symbol.hasInstance`][] +for both base classes by overriding [`Symbol.hasInstance`][]. Custom Duplex streams *must* call the `new stream.Duplex([options])` constructor and implement *both* the `readable._read()` and From 35c92b9a1aa6253ef68f9586b3a7585b01335d1f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 28 Sep 2016 20:24:02 +0200 Subject: [PATCH 3/5] =?UTF-8?q?[squash]=20by=20=E2=86=92=20due=20to?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- doc/api/stream.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index 996d0db8346d34..1eb137ef9cf17f 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1560,7 +1560,8 @@ to extending the `stream.Readable` *and* `stream.Writable` classes). *Note*: The `stream.Duplex` class prototypically inherits from `stream.Readable` and parasitically from `stream.Writable`, but `instanceof` will work properly -for both base classes by overriding [`Symbol.hasInstance`][]. +for both base classes due to overriding [`Symbol.hasInstance`][] +on `stream.Writable`. Custom Duplex streams *must* call the `new stream.Duplex([options])` constructor and implement *both* the `readable._read()` and From 297b6fd353d6c51fb50af65170f4b76c8b2a4ecf Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 29 Sep 2016 09:55:30 +0200 Subject: [PATCH 4/5] [squash] feature-detect `Symbol` --- lib/_stream_writable.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index 184e52f9b6935a..2748a012946ba0 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -137,7 +137,7 @@ Object.defineProperty(WritableState.prototype, 'buffer', { // Test _writableState for inheritance to account for Duplex streams, // whose prototype chain only points to Readable. var realHasInstance; -if (Symbol.hasInstance) { +if (typeof Symbol === 'function' && Symbol.hasInstance) { realHasInstance = Function.prototype[Symbol.hasInstance]; Object.defineProperty(Writable, Symbol.hasInstance, { value: function(object) { From de72676657a01fb73c6dcc7a8045ddd4fda12581 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 29 Sep 2016 10:24:21 +0200 Subject: [PATCH 5/5] [squash] more comments! --- lib/_stream_writable.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index 2748a012946ba0..c0264df04b4872 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -141,6 +141,8 @@ if (typeof Symbol === 'function' && Symbol.hasInstance) { realHasInstance = Function.prototype[Symbol.hasInstance]; Object.defineProperty(Writable, Symbol.hasInstance, { value: function(object) { + // Trying to move the `realHasInstance` from the Writable constructor + // to here will break the Node.js LazyTransform implementation. return object._writableState instanceof WritableState; } }); @@ -152,6 +154,8 @@ if (typeof Symbol === 'function' && Symbol.hasInstance) { function Writable(options) { // Writable ctor is applied to Duplexes, too. + // `realHasInstance` is necessary because using plain `instanceof` + // would return false, as no `_writableState` property is attached. if (!(realHasInstance.call(Writable, this)) && !(this instanceof Stream.Duplex)) { return new Writable(options);