-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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.
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
doc: deprecate NaN or negative number as delay to setTimeout and setInterval #53005
base: main
Are you sure you want to change the base?
doc: deprecate NaN or negative number as delay to setTimeout and setInterval #53005
Conversation
9bc39b4
to
7603d2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd need to add an entry in deprecations.md
The
notable-change
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. |
/cc @nodejs/web-standards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
It does not, us doc-deprecating it does not mean folks can not use it, only that our documentation recommends against doing it. |
Hi @benjamingr, I understand your concern. In my opinion the current behaviour in the browser isn't ideal either. According to MDM documentation on
The change is trying to encourage or inform developers not to use We had a bug at work where one of the delay is accidentally set to I also like the suggestion to do:
I will consider doing it in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
So the plan is to doc deprecate it but never ever runtime deprecate it? |
Obviously, there’s no plan, it’s OSS after all :) |
Then I am -1 on this unless it is clear we don't runtime deprecate and 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? |
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 |
Appreciate your explanation @benjamingr 🙏 I am a bit mixed on your opinion. Please let me explain a few of my thoughts here:
|
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. |
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). |
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 |
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?
IMO our users deserve more credit, I think this is a straw-person – but I guess there's no way to know.
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
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. |
I think this is a fair take, would add some explanations might mitigate your concern? @benjamingr
Would be happy to see the impact of this if someone could run it for me please 🙏 |
Hi @benjamingr would you mind taking another look 👀 when you have some time |
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. |
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:
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 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. |
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):
|
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) |
The TSC has not reached consensus and internal discussions made the point:
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. |
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