Skip to content

Commit

Permalink
stream: fix Writable subclass instanceof checks
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
addaleax authored and evanlucas committed Oct 14, 2016
1 parent 7171bd6 commit 8c4fab0
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 3 deletions.
11 changes: 8 additions & 3 deletions lib/_stream_writable.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,10 @@ 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;
if (realHasInstance.call(this, object))
return true;

return object && object._writableState instanceof WritableState;
}
});
} else {
Expand All @@ -156,6 +157,10 @@ 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.

// Trying to use the custom `instanceof` for Writable here will also break the
// Node.js LazyTransform implementation, which has a non-trivial getter for
// `_writableState` that would lead to infinite recursion.
if (!(realHasInstance.call(Writable, this)) &&
!(this instanceof Stream.Duplex)) {
return new Writable(options);
Expand Down
16 changes: 16 additions & 0 deletions test/parallel/test-stream-inheritance.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,19 @@ assert.ok(!(readable instanceof Transform));
assert.ok(!(writable instanceof Transform));
assert.ok(!(duplex instanceof Transform));
assert.ok(transform instanceof Transform);

assert.ok(!(null instanceof Writable));
assert.ok(!(undefined instanceof Writable));

// Simple inheritance check for `Writable` works fine in a subclass constructor.
function CustomWritable() {
assert.ok(this instanceof Writable, 'inherits from Writable');
assert.ok(this instanceof CustomWritable, 'inherits from CustomWritable');
}

Object.setPrototypeOf(CustomWritable, Writable);
Object.setPrototypeOf(CustomWritable.prototype, Writable.prototype);

new CustomWritable();

assert.throws(CustomWritable, /AssertionError: inherits from Writable/);

0 comments on commit 8c4fab0

Please sign in to comment.