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

Jump to bottom

process: add process.runNextTicks() method #40088

Closed
wants to merge 13 commits into from
Closed

process: add process.runNextTicks() method #40088

wants to merge 13 commits into from

Conversation

Richienb
Copy link
Contributor

Finishes #29671

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 12, 2021
@Richienb Richienb changed the title Add process.runNextTicks() method process: add process.runNextTicks() method Sep 12, 2021
mscdex
mscdex reviewed Sep 12, 2021
View reviewed changes
doc/api/process.md Outdated Show resolved Hide resolved
Richienb and others added 3 commits September 12, 2021 17:45
@Richienb @mscdex
Update doc/api/process.md
ca914cc 
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
@Richienb @mscdex
Update doc/api/process.md
6a37928 
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
@Richienb
Update process.md
1070316
mscdex
mscdex reviewed Sep 12, 2021
View reviewed changes
doc/api/process.md Outdated Show resolved Hide resolved
Mesteery
Mesteery reviewed Sep 12, 2021
View reviewed changes
doc/api/process.md Outdated Show resolved Hide resolved
addaleax
addaleax requested changes Sep 12, 2021
View reviewed changes
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.

I think we would need a very strong reason if we wanted to make this public API, given that it is intentional that this is neither part of the language nor other runtime environments like browsers.

@Richienb
Copy link
Contributor Author

If #40054 is not possible, this is the next best thing to achieve the same result.

@addaleax
Copy link
Member

@Richienb I think this PR would go waaaaay further than what is being suggested in #40054. This would enable running code that behaves as asynchronous in every JS environment as synchronous code; that’s a big footgun.

Also, I just commented on #40054, it is possible to do this with existing APIs.

@jasnell
Copy link
Member

jasnell commented Sep 12, 2021

I'm definitely in agreement with @addaleax ... really not convinced this is something we should do.

ronag
ronag requested changes Sep 15, 2021
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

This seems dangerous and easily misused. Do we have a motivating example for why this would be needed?

@Richienb
Copy link
Contributor Author

Since #40054 has been proven possible, this PR is no longer needed.

@Richienb Richienb closed this Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax requested changes

@ronag ronag ronag requested changes

@mscdex mscdex Awaiting requested review from mscdex

@Mesteery Mesteery Awaiting requested review from Mesteery

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Richienb @addaleax @jasnell @mscdex @ronag @Mesteery @nodejs-github-bot