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

lib: add --deprecate-soon and util.deprecateSoon() #9483

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Nov 6, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

lib, src

Description of change

This adds a non-intrusive mechanism for indicating that an API is expected to be deprecated "soon" where "soon" means at least the next semver-major Node.js release. The DeprecateSoonWarning is only emitted if the --deprecate-soon command line option is used which means the option will have zero impact for most situations. For those developers who wish to be proactively alerted to pending deprecations, use of the --deprecate-soon option can be used in testing/CI environments to determine which APIs may end up deprecated in the future.

It should be noted that a "deprecate soon" warning is just a heads up, and that a decision can be made later NOT to deprecate the API if the change is deemed to be too intrusive.

This adds a non-intrusive mechanism for indicating that an API is
expected to be deprecated "soon" where "soon" means at least the
next semver-major Node.js release. The `DeprecateSoonWarning` is
only emitted if the `--deprecate-soon` command line option is
used which means the option will have zero impact for most
situations. For those developers who wish to be proactively
alerted to pending deprecations, use of the `--deprecate-soon`
option can be used in testing/CI environments to determine
which APIs may end up deprecated in the future.

It should be noted that a "deprecate soon" warning is just a
heads up, and that a decision can be made later NOT to
deprecate the API if the change is deemed to be too
intrusive.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module. labels Nov 6, 2016
@jasnell
Copy link
Member Author

jasnell commented Nov 6, 2016

@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label Nov 6, 2016
@jasnell
Copy link
Member Author

jasnell commented Nov 6, 2016

Marking this in progress because I would like to get some discussion and input about whether this will be useful before landing.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 6, 2016

I don't think anyone outside of core will ever use such a flag, but maybe I'm wrong. I'd personally rather just use documentation deprecations to try to accomplish this.

@jasnell
Copy link
Member Author

jasnell commented Nov 6, 2016

btw, sorry this wasn't clear, but the deprecateSoon() method is added to internal/util.js...

@mscdex
Copy link
Contributor

mscdex commented Nov 6, 2016

-1 "soon" is too vague and if the target use case is giving people a chance to change their code in advance, then the possibility of the deprecation being reverted means people could be worrying about nothing.

Besides, I thought the whole idea behind the already-implemented warning system was to actively let people more know about changes coming "soon?"

@Fishrock123
Copy link
Contributor

+1 for --deprecated-in-docs

@jasnell
Copy link
Member Author

jasnell commented Nov 6, 2016

--deprecated-in-docs works also.

On Saturday, November 5, 2016, Jeremiah Senkpiel notifications@github.com
wrote:

+1 for --deprecated-in-docs


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9483 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2efvqTFVY57NEAoyPpWQn0WRBs8Gaks5q7UOXgaJpZM4KqbMx
.

@not-an-aardvark
Copy link
Contributor

I'm a bit confused about what it would mean to be deprecating a function "soon". If we have a warning discouraging the use of a function then it seems like the function is already deprecated, even if the warning is opt-in.

If we're just holding off on emitting a warning by default until the next semver-major release, maybe we should call the flag --show-all-deprecation-warnings instead.

@rvagg
Copy link
Member

rvagg commented Nov 8, 2016

Most of our other commandline arguments that don't take a parameter are of the form: --do-this-thing (--version and --interactive aren't and arguably the --no-X ones aren't either). --deprecated-in-docs doesn't actually tell you what it's going to do, it doesn't even hint.
We should follow the same pattern, I know it becomes verbose but "show" or "enable" get it closer. --show-doc-deprecation might actually be closer to the existing --X-deprecation options.

It's a bit late to fix the existing inconsistency we have with deprecation and warnings, we should probably have gone with deprecations, oh well.

@ChALkeR
Copy link
Member

ChALkeR commented Nov 8, 2016

+1 as a method of getting warnings on everything that is doc-deprecated (should be renamed to exclude soon and perhaps include docs, though).

-1 for introducing an additional deprecation step between the docs and runtime deprecation.

@seishun
Copy link
Contributor

seishun commented Nov 9, 2016

Is the only difference between this and hard deprecation (aka runtime deprecation) that the former is disabled by default and the latter is enabled by default?

@jasnell
Copy link
Member Author

jasnell commented Nov 10, 2016

Yes.

@seishun
Copy link
Contributor

seishun commented Nov 10, 2016

Then it should be made clearer that it's basically the opposite of --no-deprecation by placing it nearby and naming it in a similar fashion (--all-deprecation?). Although considering that the separation between "on-by-default" and "off-by-default" deprecations is rather arbitrary, I'm not sure how to document it clearly. "Show warnings for APIs that have been deprecated in the documentation." doesn't cut it, since APIs that are runtime-deprecated by default are also deprecated in the docs.

Maybe reuse the "hard/soft" terminology to distinguish "on-by-default" and "off-by-default" and reserve "runtime deprecation" for the actual warnings that depend on what the user has enabled?

@seishun
Copy link
Contributor

seishun commented Nov 11, 2016

To clarify, this is the terminology I'm proposing:

deprecation - deprecated API is marked with a scary-looking warning in the docs that tells the user to avoid it. Can be of two varieties: hard and soft.
soft deprecation - soft-deprecated API is not runtime-deprecated by default, but it can be enabled with --all-deprecation (TBD)
hard deprecation - hard-deprecated API is runtime-deprecated by default, but it can be disabled with --no-deprecation
runtime deprecation - the first time a runtime-deprecated API is used, a deprecation warning is printed to stderr

This also means that with --all-deprecation (TBD), there is no distinction between soft-deprecated and hard-deprecated API, they both output the same warnings and they both throw if --throw-deprecation is used.

cc @nodejs/collaborators I'd really like to know your opinion on this.

@sam-github
Copy link
Contributor

@jasnell I'm very much in favour of this. It solves a real problem. Node.js wants to communicate to the user base that changes are coming, but the only way to do this at the moment dumps deprecation messages on the consoles of not just module authors (who can do something about it), but every single node user of grunt, strongloop's tools, ... basically, every CLI. Its way too loud a broadcast.

Functional suggestions:

  1. maybe if NODE_ENV=test, then the "soon" deprecations should be turned on? This might make it slightly more noticeable, but also will not cause the messages to be dumped on consoles everywhere, to people who can't do anything about them.
  2. perhaps github issue numbers should be in these messages, to allow feedback on the coming deprecation, and to provide advice on how to rework code to avoid the deprecation

@seishun Its nice to have terminology, its confusing at the moment. I suggest renaming "runtime deprecated" to just "deprecated". Also, yours seems to lack a final deprecation, "actually gone"?

I would suggest different names:

  • proposed deprecation (you called "soft"): users can proactively enable console messages (and optionally, stack traces for every use of the API) to find out where APIs that have been proposed for deprecation.
  • soft deprecation: use of the API causes console messages, you need to proactively disable these messages
  • ("hard"?) deprecated: first use of the API will cause a console message, cannot be disabled
  • gone: the API is gone... this needs a name.

This seems like it implies like it would take 4 majors to get an API deprecated.

@seishun
Copy link
Contributor

seishun commented Nov 11, 2016

@sam-github

I suggest renaming "runtime deprecated" to just "deprecated".

Case in point: new Buffer is currently deprecated, but it doesn't display a warning.

proposed deprecation (you called "soft"):

If a deprecation is "proposed", then it's just that, a proposal. It doesn't make sense to add any warnings for something that may or may not happen.

Going back to the new Buffer example, it's a different case. It's definitely deprecated now and it's not getting un-deprecated any time soon. What's being discussed is whether and when to display a warning by default when new Buffer is used.

soft deprecation: use of the API causes console messages, you need to proactively disable these messages

("hard"?) deprecated: first use of the API will cause a console message, cannot be disabled

So introduce an extra deprecation stage before possible removal? I'm not sure if there is any point in that.

gone: the API is gone... this needs a name.

Not sure why this needs a name. It's basically "non-existent".

@sam-github
Copy link
Contributor

@seishun Let me be clearer, given a set of words, "runtime, soft, hard", I suggest no one would be able to order them correctly, or guess their meaning. I had to read your proposal three times before I understood the difference between hard and runtime, so I appreciate the proposal to define terminology, but yours didn't work for me.

You say:

If a deprecation is "proposed", then it's just that, a proposal. It doesn't make sense to add any warnings for something that may or may not happen.

But then you suggest the word "soft" for what @jasnell described as

It should be noted that a "deprecate soon" warning is just a heads up, and that a decision can be made later NOT to deprecate the API if the change is deemed to be too intrusive.

James' description looks very much to me like what I would describe as "proposed deprecation".

Also, I'd like to point out that soft and hard deprecation are words with already defined meanings. The new state is the one this PR proposes, its the one that should get a new name.

If I understand your proposal, what used to be called "hard" is called "runtime", and what used to be called "soft" is now called "hard", and this new proposal of James' is called "soft".

Did I misunderstand? Because changing the meaning of words to be their opposite isn't going to lead to clarity, but to more confusion.

@seishun
Copy link
Contributor

seishun commented Nov 11, 2016

James' description looks very much to me like what I would describe as "proposed deprecation".

Aha, I see where the confusion is coming from. @jasnell originally proposed a --deprecate-soon option to warn about APIs there will be deprecated (I assume he meant runtime-deprecated) "soon". After some objections (see #9483 (comment)), he changed it to --deprecated-in-docs, but hasn't updated the title and description.

So what used to be called "soft" remains "soft", but now with an option to enable runtime warnings for it. I've updated my terminology proposal to hopefully make things a bit clearer.

@Trott
Copy link
Member

Trott commented Nov 11, 2016

soft deprecation and hard deprecation have caused a lot of confusion and have been ineffective as terms to use when communicating with users. This very conversation is evidence of the unsuitability of that terminology.

IMO, soft and hard should be avoided in favor of terms that are descriptive. I definitely favor docs-only, runtime, proposed, and even the clunky deprecated-soon over the vague soft and hard that we've been using for a loooong time and that people still don't understand.

@seishun
Copy link
Contributor

seishun commented Nov 11, 2016

@Trott if we add an option to warn on uses of all deprecated (whether or not there is a runtime warning by default) API, then we need to come up with terminology to distinguish API with "on-by-default" runtime warning (currently known as "hard deprecation" or "runtime deprecation") and API with "off-by-default" runtime warning (currently known as "soft deprecation" or "docs-only deprecation", but the latter would no longer be an adequate name after the option is added).

docs-only doesn't work because using the new option will make it warn in runtime. runtime could refer to both "on-by-default" and "off-by-default". proposed and deprecated-soon are unclear as to what they refer to, see #9483 (comment) and #9483 (comment).

@mhdawson
Copy link
Member

I agree with @Trott soft/hard is not intuitive. Something that tells me where it is in the process and the impact it will have on me would be much better.

@seishun
Copy link
Contributor

seishun commented Nov 12, 2016

@jasnell could you please update the issue title and description to make it clear that it's for adding a runtime warning to API that's already deprecated, but currently docs-only? Clearly people are getting confused. (I'll delete this comment afterwards)

@jasnell
Copy link
Member Author

jasnell commented Feb 9, 2017

Given the addition of the updated deprecation policy I'm going to hold off on this and close this PR. We can revisit this later if necessary

@jasnell jasnell closed this Feb 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.