From 58c813fb37a1024690fddff6ad4af181d01b0524 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 6 Oct 2019 12:07:51 +0200 Subject: [PATCH 1/3] doc: notes about forwarding stream options It is a common and unfortunate pattern to simply just forward any and all options into the base constructor without taking into account whether these options might conflict with basic stream assumptions. --- doc/api/stream.md | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index 5fd73cb2fb35d9..d26774dc5f1183 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1645,13 +1645,24 @@ parent class constructor: const { Writable } = require('stream'); class MyWritable extends Writable { - constructor(options) { - super(options); + constructor({ highWaterMark, ...options }) { + super({ + highWaterMark, + autoDestroy: false, + emitClose: true + }); // ... } } ``` +When extending streams it is important to keep in mind what options the user +can and should provide before forwarding these to the base constructor. For +example if the implementation makes assumptions in regard to e.g. the +`autoDestroy` and `emitClose` options, it becomes important to not allow the +user to override these. It is therefore recommend to be explicit about what +options are forwarded instead of implicitly forwarding all options. + The new stream class must then implement one or more specific methods, depending on the type of stream being created, as detailed in the chart below: From e919ab68cc9e5e76ba60f11282df94537a033893 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 8 Oct 2019 20:08:42 +0200 Subject: [PATCH 2/3] fixup: autoDestroy --- 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 d26774dc5f1183..25d949285e1b97 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1648,7 +1648,7 @@ class MyWritable extends Writable { constructor({ highWaterMark, ...options }) { super({ highWaterMark, - autoDestroy: false, + autoDestroy: true, emitClose: true }); // ... From 50ee05490b3136dd0756e9a40d70cbcbca424181 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 9 Oct 2019 07:56:32 +0200 Subject: [PATCH 3/3] Apply suggestions from code review Co-Authored-By: Rich Trott --- doc/api/stream.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index 25d949285e1b97..0bc52c175414f9 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1656,11 +1656,11 @@ class MyWritable extends Writable { } ``` -When extending streams it is important to keep in mind what options the user +When extending streams, it is important to keep in mind what options the user can and should provide before forwarding these to the base constructor. For -example if the implementation makes assumptions in regard to e.g. the +example, if the implementation makes assumptions in regard to e.g. the `autoDestroy` and `emitClose` options, it becomes important to not allow the -user to override these. It is therefore recommend to be explicit about what +user to override these. It is therefore recommended to be explicit about what options are forwarded instead of implicitly forwarding all options. The new stream class must then implement one or more specific methods, depending