GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: clarify fd behaviour with {read,write}File by thefourtheye · Pull Request #23706 · nodejs/node

Conversation

@thefourtheye

@thefourtheye thefourtheye commented Oct 17, 2018

Copy link
Copy Markdown
Contributor
Checklist
  • documentation is changed or added
  • commit message follows commit guidelines

Ref: #23433 (comment)

cc @nodejs/fs @nodejs/documentation @Trott

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@thefourtheye
doc: clarify fd behaviour with {read,write}File
351d9df
@nodejs-github-bot

nodejs-github-bot commented Oct 17, 2018

Copy link
Copy Markdown
Collaborator

@thefourtheye build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1271/pipeline
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to Node.js documentation. fs Issues and PRs related to file-system APIs and the fs module. labels Oct 17, 2018
@thefourtheye thefourtheye mentioned this pull request Oct 17, 2018
fs: behaviour of readFile and writeFile with file descriptors #23433
Closed
addaleax
addaleax approved these changes Oct 17, 2018
View reviewed changes
lpinca
lpinca approved these changes Oct 17, 2018
View reviewed changes
vsemozhetbyt
vsemozhetbyt approved these changes Oct 17, 2018
View reviewed changes

@vsemozhetbyt vsemozhetbyt left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

With nit suggestions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/fs.md Outdated
automatically.
3. The reading will begin at the current position. If the file size is
10 bytes and if six bytes are already read with this file descriptor, then
`readFile` will return only the rest of the four bytes.

@vsemozhetbyt vsemozhetbyt Oct 17, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nit:

Suggested change
`readFile` will return only the rest of the four bytes.
`readFile()` will return only the rest of the four bytes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/fs.md Outdated
automatically.
3. The writing will begin at the beginning of the file. If the file size
is 10 bytes and if six bytes are written with this file descriptor, then
`writeFile` will return six bytes newly written and four bytes from the file.

@vsemozhetbyt vsemozhetbyt Oct 17, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This seems a bit confusing as writeFileSync() returns undefined and writeFile() transfers only error to the calback. Maybe something like this this?

Suggested change
`writeFile` will return six bytes newly written and four bytes from the file.
the file contents would be six bytes newly written and four bytes from the file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vsemozhetbyt

vsemozhetbyt commented Oct 17, 2018

Copy link
Copy Markdown
Contributor

Should we also update filehandle.readFile(), filehandle.writeFile(), fsPromises.readFile(), and fsPromises.writeFile() docs? Or this should be done after the #23709 ?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thefourtheye

thefourtheye commented Oct 17, 2018

Copy link
Copy Markdown
Contributor Author

@vsemozhetbyt If #23709 lands, behavior of writeFile has to be updated here.
vsemozhetbyt and gdams reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thefourtheye
address @vsemozhetbyt's comments
5370c14
@vsemozhetbyt vsemozhetbyt added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 19, 2018
trivikr
trivikr approved these changes Oct 19, 2018
View reviewed changes
thefourtheye added 3 commits October 20, 2018 04:21
@thefourtheye
changes because of nodejs#23709
b5a440c
@thefourtheye
group the relevant parts
d44bf9f
@thefourtheye
update fs promisified functions' behaviour
121e1dd
@thefourtheye

thefourtheye commented Oct 19, 2018

Copy link
Copy Markdown
Contributor Author

Now that #23709 is gearing up to land, I want this PR also to be landed along with that. So, I have made changes to the writeFile* documentation according to that. @nodejs/fs and the reviewers PTAL again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vsemozhetbyt
vsemozhetbyt reviewed Oct 19, 2018
View reviewed changes
Comment thread doc/api/fs.md Outdated
3. The writing will begin at the current position. Basically, if there are
multiple write calls to a file descriptor and then a `writeFile()` call is
made with the same file descriptor, the data would have been appended.
For example, if we did something like this

@vsemozhetbyt vsemozhetbyt Oct 19, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Maybe this line can be omitted as self-evident.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vsemozhetbyt

vsemozhetbyt commented Oct 19, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I am in doubt for these fragments of write counterparts:

the data would have been appended.

the data will be written from the current position till the end of the file.

This may be true only if the current position is the end of the file or the newly written data is less then the old rewritten data.

Say, we have HelloWorld and the current position is after Hello. If we write 1, would not the content be Hello1orld? If so, neither of notes is true,
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thefourtheye

thefourtheye commented Oct 20, 2018

Copy link
Copy Markdown
Contributor Author

@vsemozhetbyt Once #23709 lands, the behaviour will become like this

➜  node git:(fix-fs-writeFile-with-fd) echo "Hello World" > /tmp/test
➜  node git:(fix-fs-writeFile-with-fd) cat ~/Desktop/Scratch/Test.js
'use strict';
const fs = require('fs');
const fileName = '/tmp/test';
const fd = fs.openSync(fileName, 'w');

fs.writeFile(fd, 'Hello', function(err) {
  if (err) {
    throw err;
  }
  fs.writeFile(fd, 'World', function(err) {
    if (err) {
      throw err;
    }
    console.log(fs.readFileSync(fileName).toString());
  });
});
➜  node git:(fix-fs-writeFile-with-fd) ./node ~/Desktop/Scratch/Test.js
HelloWorld
➜  node git:(fix-fs-writeFile-with-fd)

Even if the file has contents, the first write will always truncate the file. So, it will always seem like appending only.
vsemozhetbyt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vsemozhetbyt

vsemozhetbyt commented Oct 20, 2018

Copy link
Copy Markdown
Contributor

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1316/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Oct 22, 2018
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ChALkeR
ChALkeR reviewed Oct 24, 2018
View reviewed changes
Comment thread doc/api/fs.md Outdated
1. Any specified file descriptor has to support reading.
2. If a file descriptor is specified as the `path`, it will not be closed
automatically.
3. The reading will begin at the current position. If the file size is

@ChALkeR ChALkeR Oct 24, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Clarify that the If ... is an example?
Something like E.g., if ... or For example, if ....

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@thefourtheye thefourtheye Oct 24, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ack.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ChALkeR
ChALkeR reviewed Oct 24, 2018
View reviewed changes
Comment thread doc/api/fs.md Outdated

The `FileHandle` has to support reading.

**Note:** If one or more `filehandle.read()` calls are made on a file handle

@ChALkeR ChALkeR Oct 24, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We deliberately don't use **Note:** anymore afaik?

See #18592.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Trott Trott Oct 24, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It's not official, but there has definitely been an effort to use it much less. In this case, I think It can be dropped and you can just say what you have to say:

If one or more `filehandle.read()` calls are made on a file handle

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ChALkeR reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@thefourtheye thefourtheye Oct 24, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Makes sense. I removed the Note now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ChALkeR
ChALkeR reviewed Oct 24, 2018
View reviewed changes
Comment thread doc/api/fs.md Outdated
It is unsafe to use `filehandle.writeFile()` multiple times on the same file
without waiting for the `Promise` to be resolved (or rejected).

**Note:** If one or more `filehandle.write()` calls are made on a file handle

@ChALkeR ChALkeR Oct 24, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ditto.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@thefourtheye thefourtheye Oct 24, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ack

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ChALkeR
ChALkeR approved these changes Oct 24, 2018
View reviewed changes

@ChALkeR ChALkeR left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM with nits.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhdawson
mhdawson approved these changes Oct 24, 2018
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM once comments are addressed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
thefourtheye added 2 commits October 24, 2018 18:36
@thefourtheye
Revert "changes because of nodejs#23709"
9975e9a 
This reverts commit b5a440c.
@thefourtheye
address review comments
ef290a7
@thefourtheye

thefourtheye commented Oct 24, 2018

Copy link
Copy Markdown
Contributor Author

I reverted the changes necessary for #23709 and addressed the review comments. Reviewers, PTAL again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thefourtheye

thefourtheye commented Oct 25, 2018

Copy link
Copy Markdown
Contributor Author

I'll land this by 31 October 2018, 07:30:00 (UTC time), if there are no objections or suggestions.
addaleax and ChALkeR reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Oct 25, 2018

Copy link
Copy Markdown
Member

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1368/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thefourtheye

thefourtheye commented Oct 31, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 8bd6df8.
vsemozhetbyt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thefourtheye thefourtheye closed this Oct 31, 2018
@thefourtheye
thefourtheye deleted the update-rw-file-docs-with-fd branch October 31, 2018 14:03
thefourtheye added a commit that referenced this pull request Oct 31, 2018
@thefourtheye
doc: clarify fd behaviour with {read,write}File
8bd6df8 
PR-URL: #23706
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Nov 1, 2018
@thefourtheye @targos
doc: clarify fd behaviour with {read,write}File
da494ef 
PR-URL: #23706
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Nov 1, 2018
Release proposal: v11.1.0 #23922
Merged
This was referenced Nov 2, 2018
2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#245
Closed
2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#246
Closed
2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#247
Closed
2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#248
Closed
MylesBorins pushed a commit that referenced this pull request Nov 27, 2018
@thefourtheye @MylesBorins
doc: clarify fd behaviour with {read,write}File
8180b0b 
PR-URL: #23706
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins added dont-land-on-v8.x labels Nov 27, 2018
@codebytere codebytere mentioned this pull request Nov 27, 2018
v10.13.1 proposal #24675
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@thefourtheye @rvagg
doc: clarify fd behaviour with {read,write}File
390db42 
PR-URL: #23706
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@thefourtheye @MylesBorins
doc: clarify fd behaviour with {read,write}File
9581295 
PR-URL: #23706
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
v10.14.2 proposal #24727
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments
@mcollina mcollina mcollina approved these changes
@ChALkeR ChALkeR ChALkeR approved these changes
@addaleax addaleax addaleax approved these changes
@lpinca lpinca lpinca approved these changes
@trivikr trivikr trivikr approved these changes
+2 more reviewers
@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. doc Issues and PRs related to Node.js documentation. fs Issues and PRs related to file-system APIs and the fs module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@thefourtheye @nodejs-github-bot @vsemozhetbyt @Trott @mcollina @ChALkeR @addaleax @lpinca @mhdawson @trivikr @MylesBorins
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.