process: deprecate process._tickCallback

[lholmquist]

This comment:

// A better name for this function would be runNextTicks but
// it has been exposed to the process object so we keep this legacy name
// TODO(joyeecheung): either remove it or make it public

suggests that the process._tickCallback isn't the correct name for this function, but since it is exposed on the process object, it needs to stay, and that it should be removed(or made public).

This PR deprecates the process._tickCallback funciton as well as adding the more appropriately named `process.runNextTicks to the process object.

While this PR does both, deprecate and make the renamed function public, I am open to changing this to just deprecate and not add a new, possibly unneeded function to the process object.

I am also looking for some guidance on where a test for a change like this should be added

ping @joyeecheung

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[devsnek]

this would need to start with a docs deprecation

[lholmquist]

@devsnek where would that be exactly? The "private" function _tickCallback isn't actually in the documentation, https://nodejs.org/dist/latest-v12.x/docs/api/process.html#process_process_nexttick_callback_args

[devsnek]

I think you'd have to add it like you did here.

[lholmquist]

Ok, so i'm a little confused here. I tried to follow other commits that introduced deprecations to get a sense of what needed to be done. This particular case doesn't have docs associated with it since the function is a "private" function. So perhaps it is a little different?

this would need to start with a docs deprecation

Does this mean that a first PR should be updating documentation, then another PR is adding the deprecation to code? That one liner wasn't so clear to me. Is there an actual doc that covers the steps for deprecating something. Perhaps i could add that once i've gone through the process with this PR.

Another aspect of this PR is if this deprecation/addition makes sense.

[devsnek]

I'm definitely no expert in deprecation, I'm just thinking in terms of making sure we don't have too much disruption. I believe this particular function has some non-trivial ecosystem usage.

[lholmquist]

ping @joyeecheung , i saw from twitter you were traveling, so my original ping might have missed you. Just wanted to get your thoughts on this, since you are the author of the original // TODO

[joyeecheung]

IIRC we don’t do need to do doc depreciations for undocumented stuff - we can just start with runtime documentation. However an entry in depreciations.md is a must.

I am not aware of anyone using this, but this needs an CITGM run. (On the phone right now, I will try starting one later)

[joyeecheung]

cc @nodejs/tsc @nodejs/process

Any concerns about this? Do we actually need that alias exposed instead?

[lholmquist]

thanks for the update

[jasnell]

I'm not aware of anyone using _tickCallback() directly but I wouldn't be surprised either. If CITGM comes up clean and we can't find any obvious uses, then I'd say deprecating the public API here is fine.

[BridgeAR]

I just ran Gzemnid on the top code (frequently used modules) and the output looks pretty good (I already filtered false entries):

832258	deasync-0.1.15.tgz/index.js:64:	process._tickCallback()
832258	deasync-0.1.15.tgz/index.js:70:		process._tickCallback()
1003	stdx-1.0.45.tgz/main.js:138:  process._tickCallback() 
1003	stdx-1.0.45.tgz/scripts/patch.js:28:    process._tickCallback();
1003	stdx-1.0.45.tgz/scripts/patch.js:83:              process._tickCallback();
1003	stdx-1.0.45.tgz/scripts/patch.js:89:        process._tickCallback();
1003	stdx-1.0.45.tgz/scripts/patch.js:94:      process._tickCallback();
1003	stdx-1.0.45.tgz/scripts/pnode/main.js:12:  process._tickCallback() 

Only two modules seem to actively use this. We could try to determine alternatives for deasync and to open a PR there. That way we should be fine to deprecate the property without exposing an alias publicly.

[Fishrock123]

Frankly, I don't really mind if we cause warnings in deasync because that module has terrible inherent problems and unresolvable bugs anyways.

[benjamingr]

process._tickCallback seems to be widely used in userland although I would argue that process.runNextTicks is not an API we should expose anyway (and docs is missing in this PR anyway) - the user can just let their code finish which will cause microtasks to run. Users should generally not mess with scheduling.

Userland examples are plentyful in random user code, unpopular libraries, popular libraries and just more code. I doubt these 4 examples from the first 2 GitHub search pages are that rare.

I think it needs to be documented and doc-deprecated first to begin with and I would not be comfortable with a runtime deprecation without a TSC vote and docs deprecation first.

[lholmquist]

process.runNextTicks is not an API we should expose anyway (and docs is missing in this PR anyway)

Fair point about needing docs for this. I wanted to ask the question first, if we even needed this public alternative. so if there is consensus that having this public function added, then we can add those docs in

[BridgeAR]

@benjamingr interesting that esm does not show up with Gzemnid. I'll try to check that again tomorrow. The other modules where not in the top code and did not show up because of that.

@jdalton is it maybe possible to use something else instead of process._tickCallback()?

[benjamingr]

Personally: I don't think we should have a public API for this at all. As @Fishrock123 said this is inherently dangerous.

However, I am not blocking on this. If the TSC decides that we need a public API for this then I will be fine with it. I am blocking on deprecating _tickCallback without a prior doc deprecation because I think this could cause bad user experience though-out the ecosystem. With a docs deprecation we're giving people time to change their code before we create this sort of impact.

My personal preference would be:

• Docs-deprecate _tickCallback in Node.js 13.x or even 12.x given it's undocumented at the moment.
• Add a runtime deprecation warning in 14.x
• Probably, never remove it, but maybe eventually we'll be able to.

---

@BridgeAR I assume esm was just the first popular module I found in GitHub code search. I just went through the first few pages of results. I would not assume this is something we can fix by just changing that package.

[lholmquist]

wrt to the public function, i have no strong feelings either way.

With the docs deprecation, @benjamingr just want to clarify,

1. first we(I) would need to add an entry into the existing docs for _tickCallback but label it as deprecated. Add a deprecation entry to deprecations.md as a documentation deprecation? This is a separate PR than this one
2. Wait for a release of 13 or possible 12 to happen
3. Come back to this PR to possibly merge as a runtime deprecation

[benjamingr]

That sounds reasonable to me :]

[Trott]

wrt to the public function, i have no strong feelings either way.

With the docs deprecation, @benjamingr just want to clarify,

1. first we(I) would need to add an entry into the existing docs for _tickCallback but label it as deprecated. Add a deprecation entry to deprecations.md as a documentation deprecation? This is a separate PR than this one
2. Wait for a release of 13 or possible 12 to happen
3. Come back to this PR to possibly merge as a runtime deprecation

While it's documentation-only deprecated, you could also add code so that it was one of the documentation-only deprecations that becomes a runtime deprecation when Node.js is run with the --pending-deprecation flag or the NODE_PENDING_DEPRECATION environment variable.

[lholmquist]

While it's documentation-only deprecated, you could also add code so that it was one of the documentation-only deprecations that becomes a runtime deprecation when Node.js is run with the --pending-deprecation flag or the NODE_PENDING_DEPRECATION environment variable.

That sounds reasonable. Would this still be a separate PR?

[Trott]

While it's documentation-only deprecated, you could also add code so that it was one of the documentation-only deprecations that becomes a runtime deprecation when Node.js is run with the --pending-deprecation flag or the NODE_PENDING_DEPRECATION environment variable.

That sounds reasonable. Would this still be a separate PR?

It could be either in this PR or in a separate PR. I have a slight preference for doing it in a separate PR, but either would be fine in my opinion.

[lholmquist]

I've just created this PR, #29781, that starts with a documentation only deprecation.

Leaving this PR open, so i can come back to it, when/if necessary

[benjamingr]

Hey, thanks a ton for following up on this! I have removed my blocking code review and now I am not sure what the process is for landing this.

@Trott since the docs deprecation was uncontroversial but runtime deprecation might be very controversial - what should we do in order to solicit feedback? I am hesitant to add the tsc-agenda tag but I would like to solicit feedback.

If @Fishrock123 and @BridgeAR think this should land (on v13) as a runtime deprecation I'm game.

---

LGTMing the actual code change.

[Trott]

@Trott since the docs deprecation was uncontroversial but runtime deprecation might be very controversial - what should we do in order to solicit feedback? I am hesitant to add the tsc-agenda tag but I would like to solicit feedback.

Since it will require two TSC approvals anyway, maybe let's start by pinging @nodejs/tsc. If that fails to attract adequate attention, we can always ping more widely....

[lholmquist]

Just wanted to ping here to see if i needed to do anything else with this PR atm

[benjamingr]

This needs a second TSC approval for it to land (or a request for changes) @nodejs/tsc

[addaleax]

At the risk of repeating an annoying opinion I’ve already voiced in the past: I feel it’s unnecessary to runtime-deprecate something that’s a direct alias of a documented, public API, because to me that means there’s no point in changing or removing that alias in the future.

(That’s basically what’s kept me from approving this.)

[addaleax]

Actually, this needs docs for process.runNextTicks(), right? I don’t mind exposing it, but we should document exactly what it does.

[lholmquist]

wrt to documenting process.runNextTicks, i'll add some docs for it. i guess i was just waiting to see what the consensus was going to be about the new public function

[targos]

I agree with @addaleax, keeping the alias "forever" costs us almost nothing, while emitting a runtime deprecation forces our users to take time to migrate to the other name without giving them something in return.

[joyeecheung]

When there is evidence that the API has been used in the wild (and there is, thanks @benjamingr for looking into it), I think we should hold off deprecating something until we are certain that the usage drops to a point where it's safe to deprecate it. Chrome has a similar practice with telemetry (remove something when the usage drops to 0.000?% of the Web but don't do anything disruptive if it's used by more than ?% of the Web) when they try to deprecate things.

[lholmquist]

sounds good. I'm good with just waiting until this doesn't affect users. Just was asking to see if i needed to do anything else. Thanks!

[chris-codaio]

Pardon my naivety: If this is removed, what's the recourse for implementing a fake timers drain for testing when native promises are in the mix?

[jasnell]

Closing due to the lack of continued activity on this. Can reopen if it is picked back up