stream: proper `instanceof` for `Writable`s #8834

addaleax · 2016-09-28T17:53:51Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

streams

Description of change

Use [Symbol.hasInstance]() to return true when asking for new Duplex() instanceof Writable.

Ref: #8830 (comment)

/cc @nodejs/streams

Use `[Symbol.hasInstance]()` to return `true` when asking for `new Duplex() instanceof Writable`.

italoacasas · 2016-09-28T17:55:25Z

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`][]


@italoacasas done :)

Fishrock123 · 2016-09-28T18:06:13Z

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`][].


You’re the native speaker, so I’m going to go with that. ;)

mcollina

I think this PR can be slimmed down a bit, but I might be wrong.
I think that this is semver-minor, but we might discuss if it's semver-major instead.

mcollina · 2016-09-29T07:20:09Z

lib/_stream_writable.js

+// Test _writableState for inheritance to account for Duplex streams,
+// whose prototype chain only points to Readable.
+var realHasInstance;
+if (Symbol.hasInstance) {


We should feature-detect that Symbol exists to avoid a regexp in readable-stream. Some of our supported platforms there do not have symbols yet.

Oh, right. Done!

mcollina · 2016-09-29T07:27:20Z

lib/_stream_writable.js

-  if (!(this instanceof Writable) && !(this instanceof Stream.Duplex))
+  // Writable ctor is applied to Duplexes, too.
+  if (!(realHasInstance.call(Writable, this)) &&
+      !(this instanceof Stream.Duplex)) {


Why do we need to change this block? I think we can probably leave as it was before here.
As I understand this PR, all the awesomeness is implemented by overriding Writable.hasInstance.
So, can we remove realHasInstance?

Leaving as it is will lead to infinite recursion, because this instanceof Writable returns false before the constructor is run, so the constructor just calls itself over and over again. And adding an extra realHasInstance check inside of the symbol-based override would break our fancy LazyTransform logic.

Feel free to play around with this, but I am afraid this would be necessary.

How about we solve this by adding a check on Writable.hasInstance like: Function.prototype[Symbol.hasInstance].call(Writable, this) || this._writableState instanceof WritableState? I think that will reduce our cruft.

I would like to avoid adding one more variable to support older node variables :(

I think that will reduce our cruft.

@mcollina Yeah, me too, so I tried it – that’s what breaks LazyTransform, because it invokes the Writable constructor from inside a _writableState getter. :(

Let's add a comment for that, because it is definitely not obvious.

@calvinmetcalf what do you think?

I’ve added comments that hopefully clarify things a bit. :)

mcollina · 2016-09-29T07:29:16Z

BTW, I did not know this was even possible. Good job @addaleax.

addaleax · 2016-09-29T07:57:48Z

@mcollina updated, ptal!

mcollina · 2016-09-29T08:27:02Z

I'm LGTM, but we should wait @calvinmetcalf.

mcollina · 2016-09-29T08:27:26Z

I think this is semver-minor.

addaleax · 2016-09-30T14:42:11Z

CI: https://ci.nodejs.org/job/node-test-commit/5381/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/402/

mcollina · 2016-09-30T14:47:32Z

I would really like to have another @nodejs/streams folk review this.
Anyone around?

jasnell

LGTM!
nice!

calvinmetcalf · 2016-09-30T20:03:13Z

lgtm

mafintosh · 2016-09-30T20:11:00Z

👍 Thanks for making this compatible with old node versions for readable-stream as well.

mafintosh · 2016-09-30T20:16:53Z

(Just noting we should be careful not to promote streams instanceof checks outside of core as they'll break with modules using readable-stream)

addaleax · 2016-09-30T20:17:52Z

👍 Thanks for making this compatible with old node versions for readable-stream as well.

Just to be clear, this isn’t going to work on older node versions, unfortunately; it’s just not going to crash. (I assume you know that but it may not be obvious to everyone).

calvinmetcalf · 2016-09-30T20:40:17Z

for the record foo instaceof Stream does work with readable stream but that's it

bengl · 2016-10-01T16:48:21Z

@mafintosh

(Just noting we should be careful not to promote streams instanceof checks outside of core as they'll break with modules using readable-stream)

Maybe it's worth having similar Symbol.hasInstance functions for Readable, etc. so that instanceof checks outside of core are more usable? (In a separate PR.)

mcollina · 2016-10-02T11:06:33Z

@bengl yes. But they will still be useless. readable-stream has a completely different inheritance chain, so users cannot expect to instanceof Writable (or Duplex) and also check if it comes from readble-stream.

This change will be useful when interacting with core or core-provided streams.

addaleax · 2016-10-08T11:21:27Z

I’d like to land this on Monday if there are no objections.

Fishrock123 · 2016-10-10T16:02:57Z

I'd like to get this into v6.8.0

addaleax · 2016-10-10T16:04:15Z

I don’t think there are any reasons not to land this now, so I’ll do that?

addaleax · 2016-10-10T16:14:50Z

Landed in 2a4b068

Use `[Symbol.hasInstance]()` to return `true` when asking for `new Duplex() instanceof Writable`. PR-URL: #8834 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Calvin Metcalf <calvin.metcalf@gmail.com>

* fs: - `SyncWriteStream` now inherits from `Stream.Writable`. (Anna Henningsen) #8830 - Practically, this means that when stdio is piped to a file, stdout and stderr will still be `Writable` streams. - `fs.existsSync()` has been undeprecated. `fs.exists()` remains deprecated. (Dan Fabulich) #8364 * http: `http.request()` now accepts a `timeout` option. (Rene Weber) #8101 * module: The module loader now maintains its own realpath cache. (Anna Henningsen) #8100 * npm: Upgraded to 3.10.8 (Kat Marchán) #8706 * stream: `Duplex` streams now show proper `instanceof Stream.Writable`. (Anna Henningsen) #8834 * timers: Improved `setTimeout`/`Interval` performance by up to 22%. (Brian White) #8661 PR-URL: #9034

jcready · 2016-10-13T21:13:58Z

FYI this breaks Babel when trying to create a class that extends stream.Writable because of Babel's _classCallCheck. So this is a breaking change.

import { Writable } from 'stream'
class Example extends Writable {}
export default new Example()

Compiled:

'use strict';

Object.defineProperty(exports, "__esModule", {
  value: true
});

var _stream = require('stream');

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

function _possibleConstructorReturn(self, call) { if (!self) { throw new ReferenceError("this hasn't been initialised - super() hasn't been called"); } return call && (typeof call === "object" || typeof call === "function") ? call : self; }

function _inherits(subClass, superClass) { if (typeof superClass !== "function" && superClass !== null) { throw new TypeError("Super expression must either be null or a function, not " + typeof superClass); } subClass.prototype = Object.create(superClass && superClass.prototype, { constructor: { value: subClass, enumerable: false, writable: true, configurable: true } }); if (superClass) Object.setPrototypeOf ? Object.setPrototypeOf(subClass, superClass) : subClass.__proto__ = superClass; }

var Example = function (_Writable) {
  _inherits(Example, _Writable);

  function Example() {
    _classCallCheck(this, Example);

    return _possibleConstructorReturn(this, (Example.__proto__ || Object.getPrototypeOf(Example)).apply(this, arguments));
  }

  return Example;
}(_stream.Writable);

exports.default = new Example();

Fishrock123 · 2016-10-13T21:22:41Z

Sounds like a bug?

mcollina · 2016-10-13T21:29:28Z

I fear we merged this too early.

This is the error:


/Users/matteo/e.js:9
function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }
                                                                                            ^

TypeError: Cannot call a class as a function
    at _classCallCheck (/Users/matteo/e.js:9:99)
    at new Example (/Users/matteo/e.js:19:5)
    at Object.<anonymous> (/Users/matteo/e.js:27:19)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:394:7)

mcollina · 2016-10-13T21:34:02Z

A note to add:

const Writable = require('stream').Writable
class Example extends Writable {}
new Example()

Does not error.

The error happens because of what @addaleax and myself discussed here: https://github.com/nodejs/node/pull/8834/files#diff-1915a7b992a3012b177dd855fa3477e0R144.
I think a nicer fix should involve LazyTransform as well.

I think we should revert this asap. Babel is on everyone's desk.

As a side note, I think this is the polyfill fault, and it is not semver-major. Node.js is not responsible of the code generated by the transpiler, as the code works smoothly with the language itself.
However, the polyfill has no other way to implement it. So, it's a bug that impacts a potentially huge number of user, and we need to fix it.

@jcready can you please suggest a babelified library that we can put under CITGM?

addaleax · 2016-10-13T21:58:53Z

I think this might be an actual bug in my PR and I think I know where that comes from… should have a PR up in a few minutes. Sorry for the trouble.

2a4b068 introduced a regression in where checking `instanceof` would fail for `Writable` subclasses inside the subclass constructor, i.e. before `Writable()` was called. Also, calling `null instanceof Writable` or `undefined instanceof Writable` would fail due to accessing the `_writableState` property of the target object. This fixes these problems. Ref: nodejs#8834 (comment)

addaleax · 2016-10-13T22:15:24Z

Proposed fix: #9088

* fs: - `SyncWriteStream` now inherits from `Stream.Writable`. (Anna Henningsen) nodejs/node#8830 - Practically, this means that when stdio is piped to a file, stdout and stderr will still be `Writable` streams. - `fs.existsSync()` has been undeprecated. `fs.exists()` remains deprecated. (Dan Fabulich) nodejs/node#8364 * http: `http.request()` now accepts a `timeout` option. (Rene Weber) nodejs/node#8101 * module: The module loader now maintains its own realpath cache. (Anna Henningsen) nodejs/node#8100 * npm: Upgraded to 3.10.8 (Kat Marchan) nodejs/node#8706 * stream: `Duplex` streams now show proper `instanceof Stream.Writable`. (Anna Henningsen) nodejs/node#8834 * timers: Improved `setTimeout`/`Interval` performance by up to 22%. (Brian White) nodejs/node#8661 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

2a4b068 introduced a regression in where checking `instanceof` would fail for `Writable` subclasses inside the subclass constructor, i.e. before `Writable()` was called. Also, calling `null instanceof Writable` or `undefined instanceof Writable` would fail due to accessing the `_writableState` property of the target object. This fixes these problems. PR-URL: #9088 Ref: #8834 (comment) Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

stream: proper instanceof for Writables

026c95a

Use `[Symbol.hasInstance]()` to return `true` when asking for `new Duplex() instanceof Writable`.

addaleax added the stream Issues and PRs related to the stream subsystem. label Sep 28, 2016

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 28, 2016

italoacasas reviewed Sep 28, 2016

View reviewed changes

[squash] fix typo

a21153d

Fishrock123 reviewed Sep 28, 2016

View reviewed changes

[squash] by → due to

35c92b9

mcollina requested changes Sep 29, 2016

View reviewed changes

[squash] feature-detect Symbol

297b6fd

[squash] more comments!

de72676

mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 29, 2016

mcollina approved these changes Sep 30, 2016

View reviewed changes

addaleax mentioned this pull request Sep 30, 2016

fs: make SyncWriteStream inherit from Writable #8830

Closed

3 tasks

jasnell approved these changes Sep 30, 2016

View reviewed changes

Fishrock123 added this to the v6.8.0 milestone Oct 10, 2016

addaleax closed this Oct 10, 2016

addaleax deleted the stream-instanceof branch October 10, 2016 16:14

Fishrock123 mentioned this pull request Oct 13, 2016

v6.8.1 patch release? #9087

Closed

addaleax mentioned this pull request Oct 13, 2016

stream: fix Writable subclass instanceof checks #9088

Closed

3 tasks

stream: proper instanceof for Writables #8834

stream: proper instanceof for Writables #8834

Conversation

addaleax commented Sep 28, 2016

Checklist

Affected core subsystem(s)

Description of change

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

mcollina left a comment

Choose a reason for hiding this comment

mcollina Sep 29, 2016 • edited Loading

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

mcollina commented Sep 29, 2016

addaleax commented Sep 29, 2016

mcollina commented Sep 29, 2016

mcollina commented Sep 29, 2016

addaleax commented Sep 30, 2016

mcollina commented Sep 30, 2016

jasnell left a comment

Choose a reason for hiding this comment

calvinmetcalf commented Sep 30, 2016

mafintosh commented Sep 30, 2016

mafintosh commented Sep 30, 2016

addaleax commented Sep 30, 2016

calvinmetcalf commented Sep 30, 2016

bengl commented Oct 1, 2016

mcollina commented Oct 2, 2016

addaleax commented Oct 8, 2016

Fishrock123 commented Oct 10, 2016

addaleax commented Oct 10, 2016

addaleax commented Oct 10, 2016

jcready commented Oct 13, 2016

Fishrock123 commented Oct 13, 2016

mcollina commented Oct 13, 2016

mcollina commented Oct 13, 2016 • edited Loading

addaleax commented Oct 13, 2016

addaleax commented Oct 13, 2016

stream: proper `instanceof` for `Writable`s #8834

stream: proper `instanceof` for `Writable`s #8834

mcollina Sep 29, 2016 •

edited

Loading

mcollina commented Oct 13, 2016 •

edited

Loading