Skip to content
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

meta: state of deprecations #19561

Closed
joyeecheung opened this issue Mar 23, 2018 · 10 comments
Closed

meta: state of deprecations #19561

joyeecheung opened this issue Mar 23, 2018 · 10 comments
Labels
meta Issues and PRs related to the general management of the project.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Mar 23, 2018

I took the advice from @Trott and @targos and lived with this in my .zshrc for a while (I also suggest people who want to runtime-deprecate Buffers try that and feel the burn, so to speak)

export NODE_PENDING_DEPRECATION=1
export NODE_OPTIONS="--trace-warnings"

And I've noticed a few issues:

  1. The tests do not pass with this, mostly because when forking processes to do message tests we don't reset the env
  2. Only two doc-only deprecations is respecting NODE_PENDING_DEPRECATION (DEP0103 and DEP0005)
  3. The deprecation code in lib/ seems to be rather ad-hoc, IMO they they should be wrapped in a helper if possible so all the environment variables and command line flags can be taken care of
  4. It is very disruptive mostly because whenever you do npm install, there will be DEP0005 warnings from duplexify, it's fixed in Avoid using deprecated Buffer API mafintosh/duplexify#17 by @ChALkeR but we have not yet updated the deps to use the new version
  5. Also, lodash, depended by eslint, sometimes emit the DEP0103 warning (depends on the code path), I am trying to fix that in internal: use util.types to migrate DEP0103 in Node.js lodash/lodash#3704

We should have a better story around our deprecations if we want our users to take them seriously. Possible enhancement:

  1. A helper mentioned in 3 above, and we should migrate our deprecation code to use that if possible
  2. Make sure doc-only deprecations respect NODE_PENDING_DEPRECATION if possible
  3. A guide in doc/guides about how to maintain the deprecations in core (I am surprised there isn't one...I think someone tried to write a guide before? Forgive me if my memory is incorrect)
  4. Create tools (e.g. codemods, eslint configs) and documentations (related: Let's start to write upgrade guides! #18836) that help people to migrate from deprecated APIs. I've written a codemod for DEP0005 before and am trying to write another one for DEP0103, we can also create a more official-like tool similar to that of course. A few good examples that I can think of
  5. Figure out how to make the deprecations work with ESM (@devsnek )

cc @nodejs/tsc

@joyeecheung
Copy link
Member Author

BTW: Probably the wrong repo to say this, but this sounds like a strategic initiative material.

@joyeecheung joyeecheung added the meta Issues and PRs related to the general management of the project. label Mar 23, 2018
@Trott
Copy link
Member

Trott commented Mar 23, 2018

The tests do not pass with this, mostly because when forking processes to do message tests we don't reset the env

Interesting. That's not my experience. Tests sill pass for me when I run make test. Are you running something else to run the tests?

@Trott
Copy link
Member

Trott commented Mar 23, 2018

Another thing about our deprecations that can be improved:

We don't supply a deprecation code when something lands on master. This means that tests for the deprecation have to check the deprecation message rather than the deprecation code. This is brittle.

The reason we don't supply a deprecation code is because we create codes incrementally. DEP005 is followed by DEP006 etc.

I'd like us to stop doing that and instead find some way to generate unique codes at the time that the code for the deprecation is written. If the deprecation code never lands on master, that's fine.

We'll still need deprecation codes to have an easily understood order so they can be found in the documentation without any trouble. So they will still need to be increasing, just not always increasing by 1.

@targos
Copy link
Member

targos commented Mar 23, 2018

We don't supply a deprecation code when something lands on master.

That's not true. Sometimes we forget to assign a code, but we are supposed to do it when the deprecation lands on master.

@Trott
Copy link
Member

Trott commented Mar 23, 2018

That's not true. Sometimes we forget to assign a code, but we are supposed to do it when the deprecation lands on master.

Thanks for the correction. The problem remains: We don't supply a deprecation code at the time the PR is opened. Tests are written to check for the message rather than the code, which results in a brittle test.

@Trott
Copy link
Member

Trott commented Mar 23, 2018

Interesting. That's not my experience. Tests sill pass for me when I run make test. Are you running something else to run the tests?

Welp, I'm wrong. Tests indeed do fail. Guess I've been doing a lit of make lint-md and make test-doc the last few days and not a whole lot of make test...

@targos
Copy link
Member

targos commented Mar 23, 2018

Tests are written to check for the message rather than the code, which results in a brittle test.

We can ask to check for the code. Test is written with DEP00XX and code is updated everywhere when the PR lands.

@Trott
Copy link
Member

Trott commented Mar 23, 2018

We can ask to check for the code. Test is written with DEP00XX and code is updated everywhere when the PR lands.

Ooh, didn't realize that! 🎉

@ChALkeR
Copy link
Member

ChALkeR commented Mar 24, 2018

Related: #18417 #18450

@Trott
Copy link
Member

Trott commented Oct 26, 2018

Seems like this conversation ran its course. Feel free to re-open if there's more to say.

@Trott Trott closed this as completed Oct 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests

4 participants