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

doc: Update emitClose default for fs streams #36653

Merged
merged 1 commit into from
Jan 2, 2021
Merged

doc: Update emitClose default for fs streams #36653

merged 1 commit into from
Jan 2, 2021

Conversation

kevinoid
Copy link
Contributor

The default for the emitClose option was changed from false to true by #31408 which landed in f0d2df4 for v14.0.0. This commit updates the fs doc to match the current behavior.

Thanks for considering,
Kevin

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Dec 28, 2020
@Trott Trott requested a review from ronag December 28, 2020 02:49
ronag
ronag requested changes Dec 28, 2020
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I think it's better to just remove any mention of emitClose from the fs docs.

@kevinoid
Copy link
Contributor Author

I think it's better to just remove any mention of emitClose from the fs docs.

I agree with that sentiment, but I'm concerned that removing it from the docs would make existing code harder to understand, especially while supported versions still use emitClose: false by default.

I actually discovered this issue while checking the docs to understand why emitClose: true was being added to options in a call to fs.createWriteStream. If it had been removed, understanding would have been more difficult.

@kevinoid kevinoid requested a review from ronag December 28, 2020 14:20
@kevinoid @Trott
doc: update emitClose default for fs streams
d548f4d 
The default for the `emitClose` option was changed from `false` to
`true` by nodejs#31408 which landed in f0d2df4 for v14.0.0.
This commit updates the fs doc to match the current behavior.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

PR-URL: nodejs#36653
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@Trott Trott force-pushed the for-upstream/fs-emitClose-docs branch from 19ca07b to d548f4d Compare January 2, 2021 18:10
@Trott Trott merged commit d548f4d into nodejs:master Jan 2, 2021
@Trott
Copy link
Member

Trott commented Jan 2, 2021

Landed in d548f4d

danielleadams pushed a commit that referenced this pull request Jan 12, 2021
@kevinoid @danielleadams
doc: update emitClose default for fs streams
d3dc124 
The default for the `emitClose` option was changed from `false` to
`true` by #31408 which landed in f0d2df4 for v14.0.0.
This commit updates the fs doc to match the current behavior.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

PR-URL: #36653
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
Merged
targos pushed a commit that referenced this pull request Aug 8, 2021
@kevinoid @targos
doc: update emitClose default for fs streams
78bb65e 
The default for the `emitClose` option was changed from `false` to
`true` by #31408 which landed in f0d2df4 for v14.0.0.
This commit updates the fs doc to match the current behavior.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

PR-URL: #36653
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
BethGriggs pushed a commit that referenced this pull request Aug 12, 2021
@kevinoid @BethGriggs
doc: update emitClose default for fs streams
73e6781 
The default for the `emitClose` option was changed from `false` to
`true` by #31408 which landed in f0d2df4 for v14.0.0.
This commit updates the fs doc to match the current behavior.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

PR-URL: #36653
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
MylesBorins pushed a commit that referenced this pull request Aug 31, 2021
@kevinoid @MylesBorins
doc: update emitClose default for fs streams
298a16a 
The default for the `emitClose` option was changed from `false` to
`true` by #31408 which landed in f0d2df4 for v14.0.0.
This commit updates the fs doc to match the current behavior.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

PR-URL: #36653
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@targos targos mentioned this pull request Sep 4, 2021
Merged
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@kevinoid @foxxyz
doc: update emitClose default for fs streams
f14f813 
The default for the `emitClose` option was changed from `false` to
`true` by nodejs#31408 which landed in f0d2df4 for v14.0.0.
This commit updates the fs doc to match the current behavior.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

PR-URL: nodejs#36653
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@ronag ronag ronag approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kevinoid @Trott @ronag @targos @nodejs-github-bot