Skip to content

Commit ee9062b

Browse files
addaleaxjasnell
authored andcommitted
stream: fix Writable subclass instanceof checks
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 <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
1 parent f5dd9bc commit ee9062b

File tree

2 files changed

+24
-3
lines changed

2 files changed

+24
-3
lines changed

lib/_stream_writable.js

+8-3
Original file line numberDiff line numberDiff line change
@@ -141,9 +141,10 @@ if (typeof Symbol === 'function' && Symbol.hasInstance) {
141141
realHasInstance = Function.prototype[Symbol.hasInstance];
142142
Object.defineProperty(Writable, Symbol.hasInstance, {
143143
value: function(object) {
144-
// Trying to move the `realHasInstance` from the Writable constructor
145-
// to here will break the Node.js LazyTransform implementation.
146-
return object._writableState instanceof WritableState;
144+
if (realHasInstance.call(this, object))
145+
return true;
146+
147+
return object && object._writableState instanceof WritableState;
147148
}
148149
});
149150
} else {
@@ -156,6 +157,10 @@ function Writable(options) {
156157
// Writable ctor is applied to Duplexes, too.
157158
// `realHasInstance` is necessary because using plain `instanceof`
158159
// would return false, as no `_writableState` property is attached.
160+
161+
// Trying to use the custom `instanceof` for Writable here will also break the
162+
// Node.js LazyTransform implementation, which has a non-trivial getter for
163+
// `_writableState` that would lead to infinite recursion.
159164
if (!(realHasInstance.call(Writable, this)) &&
160165
!(this instanceof Stream.Duplex)) {
161166
return new Writable(options);

test/parallel/test-stream-inheritance.js

+16
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,19 @@ assert.ok(!(readable instanceof Transform));
2727
assert.ok(!(writable instanceof Transform));
2828
assert.ok(!(duplex instanceof Transform));
2929
assert.ok(transform instanceof Transform);
30+
31+
assert.ok(!(null instanceof Writable));
32+
assert.ok(!(undefined instanceof Writable));
33+
34+
// Simple inheritance check for `Writable` works fine in a subclass constructor.
35+
function CustomWritable() {
36+
assert.ok(this instanceof Writable, 'inherits from Writable');
37+
assert.ok(this instanceof CustomWritable, 'inherits from CustomWritable');
38+
}
39+
40+
Object.setPrototypeOf(CustomWritable, Writable);
41+
Object.setPrototypeOf(CustomWritable.prototype, Writable.prototype);
42+
43+
new CustomWritable();
44+
45+
assert.throws(CustomWritable, /AssertionError: inherits from Writable/);

0 commit comments

Comments
 (0)