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

Node source #48357

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Node source #48357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7647954
doc: clarify mkdir() recursive behavior
Stevepurpose May 22, 2023
65a190e
Update doc/api/fs.md
Stevepurpose May 22, 2023
341eb8c
Update doc/api/fs.md
Stevepurpose May 22, 2023
2ca2074
Update doc/api/fs.md
Trott May 22, 2023
1a3c70a
Update doc/api/fs.md
Trott May 22, 2023
040f6df
Update doc/api/fs.md
Trott May 22, 2023
960e326
don't throw errors in error first callbacks,read Error-first callback…
Stevepurpose Jun 6, 2023
7e55aa3
supported only by Firefox
Stevepurpose Jun 6, 2023
d37a985
doesn't create if it exists
Stevepurpose Jun 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,12 +409,15 @@ The location information will be one of:
its dependencies.

The string representing the stack trace is lazily generated when the
`error.stack` property is **accessed**.
`error.stack` property is **accessed**.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`error.stack` property is **accessed**.
`error.stack` property is **accessed**.


The number of frames captured by the stack trace is bounded by the smaller of
`Error.stackTraceLimit` or the number of available frames on the current event
loop tick.

However `error.stack` does not work on Microsoft,works only on Firefox .


Comment on lines +418 to +420
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's off-topic – this is Node.js docs, we should not document of other runtimes behavior – and also wrong (error.stack is not defined by any standard, however every JS runtime implements it).

Suggested change
However `error.stack` does not work on Microsoft,works only on Firefox .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok if that's the standard,but why studying on w3schools it was specified under JavaScript errors ,I thought it was ok mentioning it.One time I tried logging it on my pc on a project tutorial I was following band it just never logged even though it worked on the tutorial video.

## Class: `AssertionError`

* Extends: {errors.Error}
Expand Down
24 changes: 17 additions & 7 deletions doc/api/fs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3198,20 +3198,24 @@ Asynchronously creates a directory.
The callback is given a possible exception and, if `recursive` is `true`, the
first directory path created, `(err[, path])`.
`path` can still be `undefined` when `recursive` is `true`, if no directory was
created.
created (for instance, if it was previously created).

The optional `options` argument can be an integer specifying `mode` (permission
and sticky bits), or an object with a `mode` property and a `recursive`
property indicating whether parent directories should be created. Calling
`fs.mkdir()` when `path` is a directory that exists results in an error only
when `recursive` is false.
when `recursive` is false. If `recursive` is false and the directory exists,
an `EEXIST` error occurs.

```mjs
import { mkdir } from 'node:fs';

// Creates /tmp/a/apple, regardless of whether `/tmp` and /tmp/a exist.
mkdir('/tmp/a/apple', { recursive: true }, (err) => {
if (err) throw err;
// Create ./tmp/a/apple.
mkdir('./tmp/a/apple', { recursive: true }, (err) => {
if (err) {
console.log(err)
return
}
});
```

Expand Down Expand Up @@ -3314,7 +3318,10 @@ mkdtemp(tmpDir, (err, directory) => {
// This method is *CORRECT*:
import { sep } from 'node:path';
mkdtemp(`${tmpDir}${sep}`, (err, directory) => {
if (err) throw err;
if (err) {
console.log(err)
return
}
Comment on lines +3321 to +3324
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not throw here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you were to go through the Node js docs Error class ,the section error first callbacks,talks about not throwing errors in error first callbacks,but I also noticed in the documentation it wasn't adhered to in other areas ,so I just changed a few to get a review from the maintainers ,then maybe it can be implemented all through.I think I wrote it in my commit message.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I see where you come from, that Error-first callback section of the docs would need some work as it's quite outdated and not very clear. The important piece of information is here:

node/doc/api/errors.md

Lines 178 to 180 in 7df2758

Throwing an error inside the callback **can crash the Node.js process** in most
cases. If [domains][] are enabled, or a handler has been registered with
`process.on('uncaughtException')`, such errors can be intercepted.

Crashing the process is what I would want in this case (if the operation fails, I'd expect node to exit with a non-zero exit code); how to handle the error will depend on what your use case is, and I'm not convinced we can document it better.

console.log(directory);
// Will print something similar to `/tmp/abc123`.
// A new temporary directory is created within
Expand Down Expand Up @@ -3658,7 +3665,10 @@ Asynchronously reads the entire contents of a file.
import { readFile } from 'node:fs';

readFile('/etc/passwd', (err, data) => {
if (err) throw err;
if (err) {
console.log(err)
return
}
console.log(data);
});
```
Expand Down