Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: streams emitClose #29177

Closed
ronag opened this issue Aug 17, 2019 · 2 comments
Closed

fs: streams emitClose #29177

ronag opened this issue Aug 17, 2019 · 2 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.

Comments

@ronag
Copy link
Member

ronag commented Aug 17, 2019

Allow passing emitClose: true to fs streams.

@gntem
Copy link
Contributor

gntem commented Aug 19, 2019

hello tried to make this I see there are some backwards compatibility concerns

how does this look ?

diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js
index dfff08dbbd..d8776598f7 100644
--- a/lib/internal/fs/streams.js
+++ b/lib/internal/fs/streams.js
@@ -65,7 +65,9 @@ function ReadStream(path, options) {
     options.highWaterMark = 64 * 1024;
 
   // For backwards compat do not emit close on destroy.
-  options.emitClose = false;
+  if (typeof options.emitClose !== 'boolean') {
+    options.emitClose = false;
+  }
 
   Readable.call(this, options);
 
@@ -241,7 +243,9 @@ function WriteStream(path, options) {
   options = copyObject(getOptions(options, {}));
 
   // For backwards compat do not emit close on destroy.
-  options.emitClose = false;
+  if (typeof options.emitClose !== 'boolean') {
+    options.emitClose = false;
+  }
 
   Writable.call(this, options);
 

@ronag
Copy link
Member Author

ronag commented Aug 19, 2019

instead of checking for typeof boolean I would check for undefined

gntem added a commit to gntem/node that referenced this issue Aug 19, 2019
Allow passing true for emitClose option for fs
streams.

Fixes: nodejs#29177
@Fishrock123 Fishrock123 added stream Issues and PRs related to the stream subsystem. fs Issues and PRs related to the fs subsystem / file system. labels Aug 19, 2019
@Trott Trott closed this as completed in eeea3fb Aug 23, 2019
Trott added a commit that referenced this issue Aug 23, 2019
PR-URL: #29212
Fixes: #29177
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Trott added a commit that referenced this issue Aug 23, 2019
PR-URL: #29212
Fixes: #29177
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
BridgeAR pushed a commit that referenced this issue Sep 3, 2019
Allow passing true for emitClose option for fs
streams.

Fixes: #29177

PR-URL: #29212
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
BridgeAR pushed a commit that referenced this issue Sep 3, 2019
PR-URL: #29212
Fixes: #29177
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
BridgeAR pushed a commit that referenced this issue Sep 3, 2019
PR-URL: #29212
Fixes: #29177
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants