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

[jakecastelli]

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

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-change PRs 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).

[benjamingr]

Why is it important to you that the warning is not a deprecation? How would it be different from a deprecation?

I am opposed to changes or signals that move the timers API away from the web timers standard API.

A deprecation implies Node.js will behave differently from browsers in the future (from the users' point of view) and will break WebIDL conversion rules*.

I'm afraid setinterval/setTimeout are extremely common and users will run into this in libraries users' will think will break. Even if we don't end up actually runtime deprecating it or removing the rules dictating it - it's a disservice to our community**

As for why a warning without deprecation is fair game - it's generally allowed by web standards as it's not observable but it certainly is a "grey area". Note I am not really in favor of this but as this PR has several +1s and I'm the lone -1 if people think this bug is common enough I will concede.

_{*with the caveat our timers aren't fully spec compliant in the first place}

_{**A clean CITGM run on a branch removing support ffor NaN/negaive numbers as delays for timers would change my mind as would other data indicting it's not a big a break as I suspect}

[aduh95]

A deprecation implies Node.js will behave differently from browsers in the future (from the users' point of view) and will break WebIDL conversion rules*.

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?

I'm afraid setinterval/setTimeout are extremely common and users will run into this in libraries users' will think will break.

IMO our users deserve more credit, I think this is a straw-person – but I guess there's no way to know.

it's a disservice to our community

Again, there's no way of knowing, but IMO it seems much more likely that the runtime deprecation warning would have a much more positive impact than a negative one (I can't think of any use-case for passing NaN). But again, it's just my opinion, there's probably no way to know for sure.

a warning without deprecation is fair game

I'm still unconvinced and -1 on the idea. That warning would in practice be a deprecation warning (we're recommending against using certain values with those APIs), and not following would only add confusion IMO. If we're not OK with documenting that deprecation, we should not emit a warning for it.

[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