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

doc: mark process.nextTick legacy #51280

Merged
merged 1 commit into from
Aug 12, 2024
Merged

doc: mark process.nextTick legacy #51280

merged 1 commit into from
Aug 12, 2024

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Dec 24, 2023 •
edited
Loading

I propose to documentation deprecate mark as legacy process.nextTick, for the various inconsistencies and unpredictable behaviour.
Some discussion happened here and here.
This won't probably transition to eol deprecation, but should limit the usage.

Refs: #51156

@ronag @mcollina

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Dec 24, 2023
@marco-ippolito marco-ippolito added the deprecations Issues and PRs related to deprecations. label Dec 24, 2023
@marco-ippolito marco-ippolito force-pushed the deprecate/process-nexttick branch 2 times, most recently from e3c0310 to dc6eb8d Compare December 24, 2023 15:26
@aduh95

This comment was marked as resolved.

Sign in to view
@marco-ippolito

This comment was marked as resolved.

Sign in to view
mcollina
mcollina requested changes Dec 24, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I'm -1. Let's mark it legacy instead.

@anonrig
Copy link
Member

anonrig commented Dec 24, 2023

I'm -1. Let's mark it legacy instead.

I don't think legacy will ever make a difference from the enduser perspective. What's wrong with document-only deprecating it?

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Dec 24, 2023 •
edited
Loading

I think due to the inconsistent behavior it's better to deprecate it, if legacy even if unmaintaned should still somehow reliable, which is not.
I think Legacy means, the API works but there are better and newer APIs to do the same thing, which I dont think is the case here.

@mcollina
Copy link
Member

I don't think legacy will ever make a difference from the enduser perspective. What's wrong with document-only deprecating it?

@types/node will mark as deprecated, creating a maximum amount of noise for library maintainers and a lot of unneeded churn. Moreover, there is currently no way to achieve the same result right now.

I think due to the inconsistent behavior it's better to deprecate it, if legacy even if unmaintaned should still somehow reliable, which is not.

This is incorrenct. The function is reliable in what it does. The problem stems for the usage in combination with promises and EventEmitter, as showed in #51156. When using it as originally intended, it's perfect.

Ultimately it's used everywhere in our codebase. We should not mark nextTick as doc-deprecated unless we have migrated ourselves off it.

ronag
ronag requested changes Dec 24, 2023
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.

I think we need it. Just with different behavior.

@mcollina
Copy link
Member

Event marking it legacy is likely a stretch right now, unless we have a dropped it from http, fs, streams and similar.

@ronag
Copy link
Member

ronag commented Dec 24, 2023

#51267

@marco-ippolito
Copy link
Member Author

Ultimately it's used everywhere in our codebase. We should not mark nextTick as doc-deprecated unless we have migrated ourselves off it.

@ronag Isnt what this PR #51279 goal is?

@ronag
Copy link
Member

ronag commented Dec 24, 2023

Ultimately it's used everywhere in our codebase. We should not mark nextTick as doc-deprecated unless we have migrated ourselves off it.

@ronag Isnt what this PR #51279 goal is?

The goal with that PR is to use the "correct" for node core until nextTick is "fixed". At that point we can replace deferTick with nextTick.

@marco-ippolito marco-ippolito changed the title lib: deprecate process.nextTick lib: mark process.nextTick legacy Dec 24, 2023
@marco-ippolito marco-ippolito removed the deprecations Issues and PRs related to deprecations. label Dec 24, 2023
@marco-ippolito
Copy link
Member Author

marked as legacy

@marco-ippolito marco-ippolito added the deprecations Issues and PRs related to deprecations. label Dec 24, 2023
@lpinca
Copy link
Member

lpinca commented Dec 25, 2023 •
edited
Loading

queueMicrotask() takes a single parameter so it is not a drop-in replacement.

@mcollina
Copy link
Member

On second thought, if we mark this as legacy, we should also mark legacy everywhere this is used. I don't think we should.

I have not done any analysis, but I think nextTick is also faster than queueMicrotask due to the forced closure allocation of the latter (as @lpinca said).

Let's wait until those are resolved to mark this one way or another.

ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
process: add deferTick
d56d959 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
@ronag ronag mentioned this pull request Jan 15, 2024
Closed
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
process: add deferTick
9399ba7 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
process: add deferTick
9452202 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
process: add deferTick
22d4c4e 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
process: add deferTick
de7f0c1 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
process: add deferTick
d4347e6 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
process: add deferTick
30152c2 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
process: add deferTick
7a8e1a4 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
ronag added a commit to nxtedition/node that referenced this pull request Jan 16, 2024
@ronag
process: add deferTick
ee2b31c 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
ronag added a commit to nxtedition/node that referenced this pull request Jan 16, 2024
@ronag
process: add deferTick
71086a4 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
ronag added a commit to nxtedition/node that referenced this pull request Jan 16, 2024
@ronag
process: add deferTick
a1347ed 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
ronag added a commit to nxtedition/node that referenced this pull request Jan 16, 2024
@ronag
process: add deferTick
9e07efe 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
@rluvaton
Copy link
Member

I think we should explain the problems with nextTick in the docs

mcollina
mcollina approved these changes Feb 13, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag mentioned this pull request Feb 21, 2024
Closed
6 tasks
@marco-ippolito
Copy link
Member Author

@ronag should we move this forward or can I just close it?

@marco-ippolito marco-ippolito added doc Issues and PRs related to the documentations. and removed needs-ci PRs that need a full CI run. labels Aug 12, 2024
@marco-ippolito marco-ippolito changed the title lib: mark process.nextTick legacy doc: mark process.nextTick legacy Aug 12, 2024
@marco-ippolito marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2024
@nodejs-github-bot nodejs-github-bot merged commit 624db50 into nodejs:main Aug 12, 2024
19 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 624db50

targos pushed a commit that referenced this pull request Aug 14, 2024
@marco-ippolito @targos
doc: mark process.nextTick legacy
35e1b37 
PR-URL: #51280
Refs: #51156
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
targos pushed a commit that referenced this pull request Aug 14, 2024
@marco-ippolito @targos
doc: mark process.nextTick legacy
8a8d1d2 
PR-URL: #51280
Refs: #51156
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@jasnell jasnell jasnell approved these changes

@anonrig anonrig anonrig approved these changes

@ronag ronag ronag approved these changes

Assignees
No one assigned
Labels
deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@marco-ippolito @aduh95 @anonrig @mcollina @ronag @lpinca @rluvaton @nodejs-github-bot @ShogunPanda @jasnell