doc: deprecate NaN or negative number as delay to setTimeout and setInterval

[jakecastelli]

This is a Documentation-only deprecation, the aim is to encourage developers not using NaN or negative number for delay in setTimeout and setInterval.

This is my first doc-only deprecation PR, please let me know if anything I need to fix 🙏

Refs: #46678

[aduh95]

You'd need to add an entry in deprecations.md

[github-actions]

The notable-changePRs with changes that should be highlighted in changelogs. label has been added by @aduh95.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

[aduh95]

/cc @nodejs/web-standards

[benjamingr]

Thanks for the contribution! These are stable APIs and this breaks web compatibility so I am -1 on this change as it causes setTimeout in browsers to behave more differently than Node's

I am in favor of doing the suggested change in #46596 with a CITGM run though (changing the coerced to value)

[aduh95]

this breaks web compatibility

It does not, us doc-deprecating it does not mean folks can not use it, only that our documentation recommends against doing it.

[jakecastelli]

Hi @benjamingr, I understand your concern. In my opinion the current behaviour in the browser isn't ideal either.

According to MDM documentation on setTimeout (also here):

Also note that if the value isn't a number, implicit type coercion is silently done on the value to convert it to a number — which can lead to unexpected and surprising results; see Non-number delay values are silently coerced into numbers for an example.

The change is trying to encourage or inform developers not to use NaN or negative numbers in order to avoid unexpected and surprising results . But it does not stop them from deliberately using it like @aduh95 mentioned.

We had a bug at work where one of the delay is accidentally set to 500ms in the .env which caused a bug that was a bit hard to figure out, if there was a warning message in the console it would've drawn our attention to use --trace-warnings to narrow down the scope.

I also like the suggestion to do:

I am in favor of doing the suggested change in #46596 with a CITGM run though (changing the coerced to value)

I will consider doing it in a separate PR.

[mcollina]

lgtm

[UlisesGascon]

As this is a documentation change only, I am in favor. Otherwise, it might require a deeper discussion due to compatibility discrepancies based on previous comments.

[benjamingr]

It does not, us doc-deprecating it does not mean folks can not use it, only that our documentation recommends against doing it.

So the plan is to doc deprecate it but never ever runtime deprecate it?

[aduh95]

So the plan is to doc deprecate it but never ever runtime deprecate it?

Obviously, there’s no plan, it’s OSS after all :)
If your asking for my opinion: I have nothing against the runtime deprecation proposal, as long as it’s clear that we never intend to remove it (unless the spec changes); but I think we don’t have to agree on that to land this PR.
There’s a PR open to runtime-deprecate it (Refs: #46678), you might want to object to that one instead. Landing the doc-deprecation does not mean it will be be runtime deprecated, it can stay doc-deprecated forever, or move to runtime-deprecated and never removed.

[benjamingr]

Then I am -1 on this unless it is clear we don't runtime deprecate and make it harder to write universal code

[jakecastelli]

make it harder to write universal code

I think we only emit run time warning instead of throwing an error and preventing the user from passing NaN or negative numbers. Would you mind elaborating why it makes it harder to write universal code?
(It has been an amazing experience to learn from you all btw.)

[benjamingr]

Would you mind elaborating why it makes it harder to write universal code?

Users would use libraries that work in Node.js and browsers, those libraries would start emitting a warning the user has no control over leading to frustration. The benefit itself is marginal and can be solved with a wrapper.

Contrast this with today's "confusing but consistent" behavior which isn't great but allows writing universal code (Node timers and web timers are different but are "close enough" to facilitate this)

I would be comfortable with this change in timers/promises though

[jakecastelli]

Appreciate your explanation @benjamingr 🙏 I am a bit mixed on your opinion. Please let me explain a few of my thoughts here:

libraries would start emitting a warning the user has no control over leading to frustration

• If a library tries to pass a NaN I would take a wild guess that it was most likely a mistake probably should be patched, just like we passed 500ms and it runs immediately on the next tick, and we didn't think it was this cause it and looked elsewhere.

• I took @jasnell's suggestion and made it only emit once per process to make sure that libraries used NaN or negative numbers would not frustrate the users too much.

"confusing but consistent" behavior

• Right now if the delay is set to the 2**32 nodejs will emit a TimeoutOverflowWarning on each function call, which the browser does not seem to do it, what I am trying to say is that the consistency is quite hard to be reached.

timers/promises

• I am onboard with the idea if the run time depreciation proposal got rejected.

[benjamingr]

If a library tries to pass a NaN I would take a wild guess that it was most likely a mistake probably should be patched

As a platform, changing the 10+ year long behavior or timers API to deviate further from the Web Timers spec and WebIDL is just not something I think we should do. I am fine with changing the behavior in APIs people don't use between Node and timers but I just don't think we should break APIs like this - even if it's code that can be rewritten better on the application side as a platform we can't break our users like that.

I will concede a (non deprecation) warning (since that wouldn't deviate further from the web timers spec) after a CITGM run showing its impact on users isn't high - which I suspect it might be.

[aduh95]

I will concede a (non deprecation) warning

Why is it important to you that the warning is not a deprecation? How would it be different from a deprecation? In both cases a runtime warning is emitted when the API is used with a specific input, I think we should stick with the usual process (i.e. I’m -1 on introducing a runtime warning without an associated deprecation ID).

[jakecastelli]

Do you think we could address this concern by adding a sentence to clarify that this is not the case?

I think this is a fair take, would add some explanations might mitigate your concern? @benjamingr

A clean CITGM run on a branch

Would be happy to see the impact of this if someone could run it for me please 🙏

[jakecastelli]

Hi @benjamingr would you mind taking another look 👀 when you have some time

[benjamingr]

IIUC, this sentence is not what you believe, but rather what you believe the users (or at least non-negligible portion of Node.js users) will interpret the deprecation, correct? Do you think we could address this concern by adding a sentence to clarify that this is not the case?

You mean we doc deprecate it but never runtime deprecate it in this case? In that point why call it a deprecation in the first place?

Deprecation implies (from a user's point of view) that a feature may eventually be removed.

[benjamingr]

I feel like I'm blocking this needlessly and causing frustration which I hate doing (since making progress is hard as is) and we should discuss this further or in the TSC agenda.

I think this is strategic:

• do we want our setTimeout to be as compatible as feasible with the browser's and work similarly in edge cases where it's possible.
• how do we feel about deprecating things without discouraging people from doing them indirectly (e.g. with a library) or intending to remove them?

My personal feelings are that we should align with browsers in APIs we both deliver when it's feasible or we already do and we should not change APIs like setTimeout's edge case behavior to be more correct at the expense of compatibility.

I also acknowledge that most/all cases of this are likely programmer errors and a warning is a good step to address it if it's a common concern, just not a deprecation warning which implies (IMO) eventual removal.

If everyone but me feels strongly we should a) deprecate this and b) it should be through a deprecation warning I ask we test the waters with a CITGM and land it in an odd-numbered version so it can be reverted quickly if the breakage I suspect will happen happens.

[benjamingr]

To not stall-block this, I've added tsc-agenda and messaged the TSC about this. If we reach consensus before the meeting I'll remove the TSC agenda label. What I believe is strategic and want to answer is the following:

What should Node.js do about "obvious" programmer errors* in Node.js (like negative/NaN to setTimeout):

• Nothing (this behaves like web browsers)
• Warn with a deprecation warning (this behavior is allowed by the specification)
• Warn with a "regular" warning (this behavior is allowed by the specification)
• Error (this behavior does not match the specification)

_*obligatory

[jasnell]

My preference would be to just emit a warning to the console if this happens but still allow it to continue working.

[benjamingr]

My preference would be to just emit a warning to the console if this happens but still allow it to continue working.

Do you have a preference to the type of warning? (deprecation warning vs "regular" warning)

[benjamingr]

The TSC has not reached consensus and internal discussions made the point:

Someone should answer the question “why hasn’t the browser started warning on it yet?”

Opinions ranged from "err" to "warn" to "nothing" and it looks like we aren't in consensus. It is in the agenda for the next meeting so we may be able to discuss it then and progress.

I think the next step is for someone to write a quick explainer with research or convince the TSC. We can also take it to a vote but I honestly personally think a tactical decision here vs. a strategic one (for web APIs) would be a mistake in coherency.

[mcollina]

I agree with @benjamingr.

[legendecas]

General -1 on a "deprecation" warning on Web APIs without relevant standard involvement. This kind of implication of eventual removal should seek for consensus on the WHATWG as well to reduce further deviation.

+1 on "regular" warning since it doesn't indicate the behavior will be eventually removed or changed.

[jakecastelli]

Hi guys, thanks for all your participation and input!

I have been thinking about this one. And I would like to write my findings (there is no doubt I will be making mistakes - as always - very happy to be corrected).

First let's answer Benjaming's question:

Someone should answer the question “why hasn’t the browser started warning on it yet?

I have spent some time reading the Living Standard (WHATWG) and in timers section 4. it says:

4. If timeout is less than 0, then set timeout to 0.

If we see MDN, it says - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number:

If the number is NaN or -0, it's returned as 0

So I guess the browser just treat NaN or negative numbers as 0 where node treats it as 1

Second, let's talk about the difference between browser and nodejs in terms of max number for delay.

In browser, I am using chrome (125.0.6422.113 (Official Build) (arm64)) at the time writing, please let me know if it behaves differently in other browsers or JS runtimes.

If we do the following:

setTimeout(()=>console.log('hello world'),2**32+5000)

In node we would see it prints "hello world" instantly (on the next tick more precisely) with a warning message:

TimeoutOverflowWarning: 4294972296 does not fit into a 32-bit signed integer. Timeout duration was set to 1.

Where in chrome - if you give it a little bit of patient (around 5000ms) it will print "hello world" without warning.

I think this is because setTimeout and setInterval in node depends on libuv implementation where the biggest delay can be stored is uint32 where the browser this does not seem to have this restriction and rolled over to 5000ms.

In summary, I agree node should behave close to the browser as much as possible, in reality there cloud always be inevitably small differences due to the implementation limitation and historical reasons etc that need to be discussed from time to time.

For this one, my proposal would be "doc depreciate it with a runtime warning but not runtime depreciate it" As we can see MDN also suggest:

if the value isn't a number, implicit type coercion is silently done on the value to convert it to a number — which can lead to unexpected and surprising results

it is not encouraged as it would lead to error or unexpected behaviour but still available to use, a runtime warning would leave room for future spec related changes (if any).

Whether you agree or disagree with me, I would appreciate your time reading this and your feedback. Off topic - when I first submitted the PR to change this about a year ago, I didn't know what WHATWG is and I have been learning dramatically, I am thankful to the opportunity learning and growing with the community ❤️

[benjamingr]

First let's answer Benjaming's question:

Hey, not my question it was raised by another TSC member :) The relevant places for where these conversions happen isn't the timers spec it's WebIDL. The fact we treat it as 1 and not 0 is probably just a historical mistake.

---

As for this whole process. I want to apologize profusely for being stuck in a month long process where feedback is hard to get. You are being extremely patient with the project and our processes and this in particular is a hard change to impact since it's strategic and not just tactical IMO (we should be consistent across web APIs).

[tniessen]

This is a Documentation-only deprecation as it only emits warning once per process and it does not change the behaviour of the functions.

Marking as semver-major because emitting a warning is not a documentation-only deprecation.

[jakecastelli]

Marking as semver-major because emitting a warning is not a documentation-only deprecation.

Hi @tniessen, when I first opened this PR I didn't know what depreciation cycle means, so my PR desc was not accurate, sorry for that. This PR specifically is only a doc depreciation, the (ref 46678) is the actual PR that emits the warning, and based on the discussion, we highly likely not going to move it to a runtime depreciation but just a regular warning. Hope it clears the confusion.

[tniessen]

Thanks for clarifying. Removing the label based on the updated description.

[benjamingr]

It looks like we are amending the process to allow this (so the runtime warning PR should eventually land and not this one): #53513

48h have passed and it has 2 TSC LGTMs so I can land it but if that's ok I'll wait another day or two to make sure there are no late objectors.

I've also checked with WHATWG (and forgot to link here): whatwg/html#10419 (comment) it seems like there are no objections to emitting a warning here from their PoV

[jakecastelli]

Since #53513 has landed, I think I should close this PR now.

Thanks everyone! If you have some time, please take a look at the runtime warning PR #46678