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.

Sign up for GitHub

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

Jump to bottom

fs: allow passing true to emitClose option #29212

Closed
wants to merge 4 commits into from
Closed

fs: allow passing true to emitClose option #29212

wants to merge 4 commits into from

Conversation

gntem
Copy link
Contributor

@gntem gntem commented Aug 19, 2019

Allow passing true for emitClose option for fs streams.

Fixes: #29177

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@gntem
fs: allow passing true to emitClose option
df1ba97 
Allow passing true for emitClose option for fs
streams.

Fixes: nodejs#29177
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Aug 19, 2019
cjihrig
cjihrig reviewed Aug 19, 2019
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tests and some sort of documentation.

addaleax
addaleax approved these changes Aug 20, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I’d also really prefer to have a test for this.

@Trott
Copy link
Member

Trott commented Aug 22, 2019

I added tests. PTAL.

@Trott
Copy link
Member

Trott commented Aug 22, 2019

I've also added documentation.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25175/

@gntem
Copy link
Contributor Author

gntem commented Aug 22, 2019 •
edited
Loading

Thanks a lot @Trott

I tried adding tests for this but the close event was emitted twice even when autoClose: false -- but then again I didn't see a condition for emitClose somewhere in the code.

cjihrig
cjihrig approved these changes Aug 22, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 23, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25198/

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 23, 2019
@Trott
Copy link
Member

Trott commented Aug 23, 2019

Landed in ceace1f...47ff44e

@Trott Trott closed this Aug 23, 2019
Trott pushed a commit that referenced this pull request Aug 23, 2019
@gntem @Trott
fs: allow passing true to emitClose option
eeea3fb 
Allow passing true for emitClose option for fs
streams.

Fixes: #29177

PR-URL: #29212
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Trott added a commit that referenced this pull request Aug 23, 2019
@Trott
test: add emitClose: true tests for fs streams
cb92126 
PR-URL: #29212
Fixes: #29177
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Trott added a commit that referenced this pull request Aug 23, 2019
@Trott
doc: add emitClose option for fs streams
47ff44e 
PR-URL: #29212
Fixes: #29177
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
@gntem @BridgeAR
fs: allow passing true to emitClose option
8f47ff1 
Allow passing true for emitClose option for fs
streams.

Fixes: #29177

PR-URL: #29212
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
@Trott @BridgeAR
test: add emitClose: true tests for fs streams
1f88ca3 
PR-URL: #29212
Fixes: #29177
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
@Trott @BridgeAR
doc: add emitClose option for fs streams
65de900 
PR-URL: #29212
Fixes: #29177
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
Merged
BridgeAR added a commit that referenced this pull request Sep 3, 2019
@BridgeAR
2019-09-04, Version 12.10.0 (Current)
471f4a9 
Notable changes:

* deps:
  * Update npm to 6.10.3 (isaacs)
    #29023
* fs:
  * Add recursive option to rmdir() (cjihrig)
    #29168
  * Allow passing true to emitClose option (Giorgos Ntemiris)
    #29212
  * Add \*timeNs properties to BigInt Stats objects (Joyee Cheung)
    #21387
* net:
  * Allow reading data into a static buffer (Brian White)
    #25436

PR-URL: #29429
BridgeAR added a commit that referenced this pull request Sep 4, 2019
@BridgeAR
2019-09-04, Version 12.10.0 (Current)
3a2e75d 
Notable changes:

* deps:
  * Update npm to 6.10.3 (isaacs)
    #29023
* fs:
  * Add recursive option to rmdir() (cjihrig)
    #29168
  * Allow passing true to emitClose option (Giorgos Ntemiris)
    #29212
  * Add \*timeNs properties to BigInt Stats objects (Joyee Cheung)
    #21387
* net:
  * Allow reading data into a static buffer (Brian White)
    #25436

PR-URL: #29429
BridgeAR added a commit that referenced this pull request Sep 4, 2019
@BridgeAR
2019-09-04, Version 12.10.0 (Current)
02c74e7 
Notable changes:

* deps:
  * Update npm to 6.10.3 (isaacs)
    #29023
* fs:
  * Add recursive option to rmdir() (cjihrig)
    #29168
  * Allow passing true to emitClose option (Giorgos Ntemiris)
    #29212
  * Add \*timeNs properties to BigInt Stats objects (Joyee Cheung)
    #21387
* net:
  * Allow reading data into a static buffer (Brian White)
    #25436

PR-URL: #29429
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell Awaiting requested review from jasnell

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fs: streams emitClose
7 participants
@gntem @Trott @nodejs-github-bot @jasnell @addaleax @cjihrig @trivikr