meta: make emitting warnings bypass deprecation cycle

[benjamingr]

Allow for emitting new warnings (in particular, in case of developer bugs) without going through a deprecation cycle.

The current phrasing is ambiguous (ignoring the joke of "we would have needed to recursively deprecate all our deprecation warnings when we emit them since we would emit new warnings")

[nodejs-github-bot]

Review requested:

• @nodejs/tsc

[mcollina]

lgtm

[tniessen]

Is there some context behind this? Is this about non-deprecation warnings? (Since deprecations with warnings are runtime deprecations, they are semver-major anyway.)

[legendecas]

Is there some context behind this? Is this about non-deprecation warnings? (Since deprecations with warnings are runtime deprecations, they are semver-major anyway.)

This is based on the discussion at #53005

[tniessen]

If the motivation is #53005, then this seems incompatible with our definition of deprecations.

If a warning is emitted even though the user did not pass --pending-deprecation, then it is not a documentation-only deprecation:

A Documentation-only deprecation is one that is expressed only within the Node.js API docs. These generate no side-effects while running Node.js.

And if it's a runtime deprecation, then its introduction is semver-major.

[benjamingr]

I'm not sure what you mean but the idea here is: "Emitting a warning is not a change that needs a deprecation cycle", the reasoning is that it does not (presumably) break users the same way as for example: changing an API or error code does.

As for semver-ness I'm fine with semver-major but honestly I'm also fine with semver-minor. The chance something like someone relying on not emitting a warning when passing NaN to setTimeout seems extremely small to me.

[tniessen]

@benjamingr What I mean is that we need a clear distinction between runtime deprecations (which emit warnings and are breaking changes) and emitting the kind of warning that you are referring to. So far, my understanding was that emitting a warning is the difference between documentation-only deprecations (non-breaking) and runtime deprecations (breaking).

[benjamingr]

@tniessen Can you suggest better phrasing? I'm fine with changing from "Emitting a warning" to "Emitting a warning about incorrect usage" or anything that you think makes the distinction clear.

I think "deprecation" is a very roundabout way to look at this though I understand why given our processes it's the one we were considering. There are many examples of Node warning on incorrect usage - warning when NODE_DEBUG is set, when there is a possible trace_event leak, when NODE_TLS_REJECT_UNAUTHORIZED is set, in the loader and many others.

[targos]

This PR is specifically for warnings that are not deprecation warnings.

[tniessen]

@targos I was pointed to #53005 as motivation for this PR, hence I (perhaps incorrectly) assumed this was about deprecations. My understanding now is that this is targeting warnings that are unrelated to any formal deprecations and that are emitted because of dangerous or likely incorrect usage of APIs. Is that accurate @benjamingr?

On a side note, and maybe I'm missing context here, are we sure that writing things to stderr should not be considered a breaking change? I could see that breaking consumers that monitor child process's error streams, but maybe that's being overly cautious.

[targos]

are we sure that writing things to stderr should not be considered a breaking change? I could see that breaking consumers that monitor child process's error streams, but maybe that's being overly cautious.

I agree with you that we should consider it a breaking change. AFAIK that's why the move from doc-deprecation to runtime-deprecation is semver-major.

[benjamingr]

My understanding now is that this is targeting warnings that are unrelated to any formal deprecations and that are emitted because of dangerous or likely incorrect usage of APIs. Is that accurate @benjamingr?

Yes that is accurate.

On a side note, and maybe I'm missing context here, are we sure that writing things to stderr should not be considered a breaking change? I could see that breaking consumers that monitor child process's error streams, but maybe that's being overly cautious.

I am not which is part of why I've been trying to solicit discussion. I am also concerned it may be overly cautious to consider users parsing stderr but I'm happy to amend the wording to make it clear this is specific to "obvious" errors.

[marco-ippolito]

if we dont consider warning a semver-major change, that means runtime deprecation can be a non semver-major change, is that correct? I guess its a side effect of this change of which I'm not against (but maybe separate discussion)

[benjamingr]

We can also still consider it semver-major but not require a deprecation cycle.

[marco-ippolito]

We can also still consider it semver-major but not require a deprecation cycle.

that sounds good to me

[benjamingr]

@tniessen can you suggest alternative phrasing?

[aduh95]

IMO we should try to keep the bar for accepting new warnings relatively high, so we should definitely keep considering new warnings as semver-major for non-experimental features. Maybe it's worth also listing Emitting a runtime warning in the list above that lists what constitute a Breaking change.

[tniessen]

We can also still consider it semver-major but not require a deprecation cycle.

@benjamingr I am not sure I am following -- what does that mean? Our docs define a deprecation cycle as follows:

A deprecation cycle is a major release during which an API has been in one of the three Deprecation levels.

Do you mean that we can emit warnings without a corresponding deprecation? That's already the case.

[aduh95]

@tniessen I think currently the following are enforced:

• Adding a new runtime warning is considered a breaking change.
• Making breaking changes to stable APIs is not possible without a deprecation cycle (with the exceptions in the list being edited by this PR).

A consequence of that is that adding new runtime non-deprecation warning not possible – unless the TSC grants a one-time exception, or if we edit the rule as this PR is doing.

[tniessen]

Ahh, thank you for explaining @aduh95, and apologies to you @benjamingr -- I misunderstood the purpose of this change. In that case, I agree with @aduh95 and would perhaps add "Emitting a runtime warning that is unrelated to a documentation-only deprecation." WDYT?

Dismissing my request for changes

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/53513
✔  Done loading data for nodejs/node/pull/53513
----------------------------------- PR info ------------------------------------
Title      meta: make emitting warnings bypass deprecation cycle (#53513)
Author     Benjamin Gruenbaum  (@benjamingr)
Branch     benjamingr:suggest-warning-exception-to-deprecations -> nodejs:main
Labels     doc, meta
Commits    1
 - meta: warnings bypass deprecation cycle
Committers 1
 - Benjamin Gruenbaum 
PR-URL: https://github.com/nodejs/node/pull/53513
Reviewed-By: Matteo Collina 
Reviewed-By: Geoffrey Booth 
Reviewed-By: Luigi Pinca 
Reviewed-By: Michaël Zasso 
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Trivikram Kamat 
Reviewed-By: Chengzhong Wu 
Reviewed-By: Marco Ippolito 
Reviewed-By: Rafael Gonzaga 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53513
Reviewed-By: Matteo Collina 
Reviewed-By: Geoffrey Booth 
Reviewed-By: Luigi Pinca 
Reviewed-By: Michaël Zasso 
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Trivikram Kamat 
Reviewed-By: Chengzhong Wu 
Reviewed-By: Marco Ippolito 
Reviewed-By: Rafael Gonzaga 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - meta: warnings bypass deprecation cycle
   ℹ  This PR was created on Wed, 19 Jun 2024 17:01:38 GMT
   ✔  Approvals: 9
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/53513#pullrequestreview-2128906325
   ✔  - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/53513#pullrequestreview-2128910041
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/53513#pullrequestreview-2128931382
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/53513#pullrequestreview-2128974321
   ✔  - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/53513#pullrequestreview-2129240782
   ✔  - Trivikram Kamat (@trivikr): https://github.com/nodejs/node/pull/53513#pullrequestreview-2129317466
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/53513#pullrequestreview-2130264011
   ✔  - Marco Ippolito (@marco-ippolito) (TSC): https://github.com/nodejs/node/pull/53513#pullrequestreview-2130571300
   ✔  - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/53513#pullrequestreview-2133978783
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/9692903443

[nodejs-github-bot]

Landed in 27f1306