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

Node source #48357

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

Node source #48357

wants to merge 9 commits into from

Conversation

Stevepurpose
Copy link
Contributor

No description provided.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. 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
Copy link
Contributor Author

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

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

Mesteery commented Jun 7, 2023

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.

@Stevepurpose
Copy link
Contributor Author

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

@Mesteery Mesteery reopened this Jun 9, 2023
Comment on lines +418 to +420
However `error.stack` does not work on Microsoft,works only on Firefox .


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.

@@ -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**.

Comment on lines +3321 to +3324
if (err) {
console.log(err)
return
}
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.

@Stevepurpose
Copy link
Contributor Author

Stevepurpose commented Jun 11, 2023 via email

@RedYetiDev
Copy link
Member

RedYetiDev commented Apr 21, 2024

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

@Stevepurpose
Copy link
Contributor Author

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

@RedYetiDev
Copy link
Member

RedYetiDev commented Apr 24, 2024

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?

@RedYetiDev RedYetiDev added the review wanted PRs that need reviews. label Apr 24, 2024
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. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants