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 — Node source by Stevepurpose · Pull Request #48357 · nodejs/node

Conversation

@Stevepurpose

@Stevepurpose Stevepurpose commented Jun 6, 2023

Copy link
Copy Markdown
Contributor

No description provided.

Sorry, something went wrong.

Uh oh!

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

All reactions
Stevepurpose and others added 9 commits May 21, 2023 22:17
@Stevepurpose @Trott
doc: clarify mkdir() recursive behavior
7647954
@Stevepurpose @Trott
Update doc/api/fs.md
65a190e
Co-authored-by: Rich Trott <[email protected]>
@Stevepurpose @Trott
Update doc/api/fs.md
341eb8c
Co-authored-by: Rich Trott <[email protected]>
@Trott
Update doc/api/fs.md
2ca2074
@Trott
Update doc/api/fs.md
1a3c70a
@Trott
Update doc/api/fs.md
040f6df
@Stevepurpose
don't throw errors in error first callbacks,read Error-first callback…
960e326
…s under Errors
@Stevepurpose
supported only by Firefox
7e55aa3
@Stevepurpose
doesn't create if it exists
d37a985
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to Node.js documentation. label Jun 6, 2023
@Mesteery Mesteery closed this Jun 6, 2023
@Mesteery Mesteery added the invalid Issues and PRs that are invalid. label Jun 6, 2023
@Stevepurpose

Stevepurpose commented Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

Would have been happier if I could learn why I am wrong by a comment to my PR

All reactions

Sorry, something went wrong.

Uh oh!

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

@Mesteery Mesteery removed the invalid Issues and PRs that are invalid. label Jun 7, 2023
@Mesteery

Mesteery commented Jun 7, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

I'm very sorry, the PR confused me (no title and commits coming from another PR), which made me think it was spam.
I imagine that the previous comments came from #48109 as you used the same branch.
However, I don't understand the changes made (960e326..d37a985)
Feel free to reopen the PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Stevepurpose

Stevepurpose commented Jun 9, 2023

Copy link
Copy Markdown
Contributor Author

Please how do I go about it,I don't think I have the permissions to do so.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Mesteery Mesteery reopened this Jun 9, 2023
aduh95
aduh95 reviewed Jun 10, 2023
View reviewed changes
Comment thread doc/api/errors.md
Comment on lines +418 to +420
However `error.stack` does not work on Microsoft,works only on Firefox .


@aduh95 aduh95 Jun 10, 2023

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.

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 .

Sorry, something went wrong.

Uh oh!

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

All reactions

@Stevepurpose Stevepurpose Jun 11, 2023

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread doc/api/errors.md

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

@aduh95 aduh95 Jun 10, 2023

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread doc/api/fs.md
Comment on lines +3321 to +3324
if (err) {
console.log(err)
return
}

@aduh95 aduh95 Jun 10, 2023

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.

Why not throw here?

Sorry, something went wrong.

Uh oh!

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

Mesteery reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@Stevepurpose Stevepurpose Jun 11, 2023

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

@aduh95 aduh95 Jun 11, 2023

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions
@aduh95 aduh95 mentioned this pull request Jun 11, 2023
doc: revise error.md introduction #48423
Merged
@Stevepurpose

Stevepurpose commented Jun 11, 2023 via email

Copy link
Copy Markdown
Contributor Author
Ok,I see.
…
On Sun, 11 Jun 2023, 07:48 Antoine du Hamel, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In doc/api/fs.md <#48357 (comment)>: > + if (err) { + console.log(err) + return + } OK I see where you come from, that Error-first callback <https://nodejs.org/api/errors.html#error-first-callbacks> section of the docs would need some work as it's quite outdated and not very clear. The important piece of information is here: https://github.com/nodejs/node/blob/7df27582bbb896ebb72697da58209976a0b4daf6/doc/api/errors.md#L178-L180 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. — Reply to this email directly, view it on GitHub <#48357 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZDIFAI65MMWSV7VILCSZMTXKVS2ZANCNFSM6AAAAAAY4GY7PU> . You are receiving this because you authored the thread.Message ID: ***@***.***>
All reactions

Sorry, something went wrong.

Uh oh!

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

@avivkeller

avivkeller commented Apr 21, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Hi! It's been a few months since any activity on this PR. If you wish to pursue it further, I suggest making sure it is still relevant and getting some reviews from the team.

Thanks for your contribution!


(CC) @nodejs/documentation

All reactions

Sorry, something went wrong.

Uh oh!

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

@Stevepurpose

Stevepurpose commented Apr 22, 2024

Copy link
Copy Markdown
Contributor Author

Thanks for the notification. I thought it has been settled by aduh95

All reactions

Sorry, something went wrong.

Uh oh!

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

@avivkeller

avivkeller commented Apr 24, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Ping @aduh95

This PR, IMO, doesn't seem like it adds to the file, and I don't think it'll do much good being merged. WDYT?

All reactions

Sorry, something went wrong.

Uh oh!

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

@avivkeller avivkeller added the review wanted PRs that need review. label Apr 24, 2024
@bjohansebas bjohansebas added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Apr 6, 2025
@github-actions

github-actions Bot commented Apr 6, 2025

Copy link
Copy Markdown
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

All reactions

Sorry, something went wrong.

Uh oh!

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

@github-actions

github-actions Bot commented May 7, 2025

Copy link
Copy Markdown
Contributor

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

All reactions

Sorry, something went wrong.

Uh oh!

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

@github-actions github-actions Bot closed this May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments

Uh oh!

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

Assignees

No one assigned

Labels

doc Issues and PRs related to Node.js documentation. review wanted PRs that need review. stalled Issues and PRs manually marked as stalled and scheduled for automatic closure.

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.

7 participants

@Stevepurpose @Mesteery @avivkeller @aduh95 @nodejs-github-bot @bjohansebas @Trott
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.