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

doc: fix fs.read arg type and setImmediate desc #12034

Closed
wants to merge 2 commits into from

Conversation

darai0512
Copy link
Contributor

Checklist
  • documentation is changed or added
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

doc

Description of change

About fs.read's 2nd argument, I think string is invalid.
https://github.com/nodejs/node/blob/v6.x-staging/src/node_file.cc#L1247-L1248

$cat fs_invalid_read_arg.js
const fs = require('fs');
fs.open('./fs_invalid_read_arg.js', 'r', (err, fd) => {
  const buffer = ''; // About 2nd argument, string is error. Buffer.alloc(1024) is ok.
  fs.read(fd, buffer, 0, 1024, 0, (err, byteRead, buffer) => {
    console.log('ok');
  })
});

$node fs_invalid_read_arg.js
fs.js:129
    throw new Error('Unknown encoding: ' + encoding);
    ^

Error: Unknown encoding: 1048576
    at assertEncoding (fs.js:129:11)
    at Object.fs.read (fs.js:654:5)
    at fs.open (/Users/daiki/workspace/fs_invalid_read_arg.js:5:6)
    at FSReqWrap.oncomplete (fs.js:123:15)

And also, about setImmediate, the timing of the execution is after timers in Event Loop of over v0.9, though it is correct in Event Loop under v0.8.
So, I deleted the description because the timing depended on Nodejs version.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 25, 2017
@vsemozhetbyt vsemozhetbyt added fs Issues and PRs related to the fs subsystem / file system. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Mar 25, 2017
@vsemozhetbyt
Copy link
Contributor

I may be wrong, but is it worth to be split into separate commits or even PRs?

@darai0512
Copy link
Contributor Author

@vsemozhetbyt
Sure, I will divide my commit into two commits.

About fs.read's 2nd argument, string is invalid.
About setImmediate, the execution timing is after timers currently.
@darai0512
Copy link
Contributor Author

@vsemozhetbyt
I divided it. PTAL.

@jasnell
Copy link
Member

jasnell commented Mar 27, 2017

@Fishrock... are you good with the Timers doc change here?

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.

^^^ That ping should probably have gone to @Fishrock123 ;)

jasnell pushed a commit that referenced this pull request Apr 4, 2017
About fs.read's 2nd argument, string is invalid.

PR-URL: #12034
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 4, 2017
About setImmediate, the execution timing is after timers currently.

PR-URL: #12034
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 4, 2017

Landed in 0844262...e9f2ec4

@jasnell jasnell closed this Apr 4, 2017
@jasnell jasnell mentioned this pull request Apr 4, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
About fs.read's 2nd argument, string is invalid.

PR-URL: nodejs#12034
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
About setImmediate, the execution timing is after timers currently.

PR-URL: nodejs#12034
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
@MylesBorins
Copy link
Contributor

is this accurate for v6.x?

@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

is this accurate for v6.x?

ping

@darai0512
Copy link
Contributor Author

@MylesBorins @gibfahn
Sorry for my late reply.
I think so, these are accurate for v6.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants