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

Deprecation Policy #1704

Closed
mikeal opened this issue May 14, 2015 · 39 comments · May be fixed by nodejs/node-convergence-archive#12
Closed

Deprecation Policy #1704

mikeal opened this issue May 14, 2015 · 39 comments · May be fixed by nodejs/node-convergence-archive#12
Labels
meta Issues and PRs related to the general management of the project.

Comments

@mikeal
Copy link
Contributor

mikeal commented May 14, 2015

Recently we've run in to issues related to deprecation and have realized we don't actually have a formal deprecation policy and even the one we are culturally enforcing now doesn't handle the breadth of use cases we have.

We should discuss and draft a deprecation policy and pull together various use cases as hypotheticals for it. Some recent issues that come to mind are:

@Fishrock123
Copy link
Contributor

Also: util.is* (which I still haven't done a doc deprecation for..), sys, etc.

@rvagg
Copy link
Member

rvagg commented May 14, 2015

I'd like to see us make 'sys' go away as a first step to making deprecation a real thing, it's been warned about for a long time now

The V8 4.4 issue is also interesting; for those not following - it's going to force us to completely remove 'smalloc' so we are put in a position of taking proper deprecation action.

@mikeal
Copy link
Contributor Author

mikeal commented May 14, 2015

part of the sys deprecation strategy should be to identify modules still relying on it and, if necessary, send them PRs.

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label May 14, 2015
@chrisdickinson
Copy link
Contributor

As part of this I'd like to try actually removing sys for v4.0.0 while doing a pre- and post- mortem of the process so we know what we're dealing with.

@Fishrock123
Copy link
Contributor

Deprecation notices may be easily maintained, but it's not really a solution. +1 to @chrisdickinson

Maybe we should do a pre-mortem first so we can gauge where this is being used the most and already send PRs / info beforehand?

Also, regarding sys, I think I remember the bigger issue being removing the things it exposes?

@chrisdickinson
Copy link
Contributor

Yep – I'll write up a pre-mortem, to include:

  • How to identify the modules that need fixing.
  • How to estimate the effectiveness of mitigation.
  • What the fixing PR process should look like.
  • Go / No-go states for removal, plus a deadline.

@jasnell
Copy link
Member

jasnell commented May 14, 2015

@mikeal ... we should work on formalizing the deprecation policy in the dev-policy document further. Some of the processes are documented there but it's been an open issue. Perhaps it would be best to keep the high level discussion of the process there so that we can focus on the specific technical items here?

@chrisdickinson
Copy link
Contributor

deprecating sys – premortem and plan of action

step 0: set goals

Our goal is to pick a minimum percentage of weekly downloads that should work
before we're willing to remove sys. Obviously ideally this would be 100%,
but our time on Earth is finite and divided between other activities that don't
involve GitHub, text editors, or the internet.

As an hypothetical example:

  • Say removing sys breaks three modules.
    • Package FOO has 10000 weekly downloads.
    • Package BAR has 1000 weekly downloads.
    • Package SHAZBOT has 10 weekly download.
  • Removing sys breaks 11010 weekly downloads.

My preference is to set this percentage between 80 and 90 percent of downloads.
We will call this the target mitigation number.

step 1: identify usage

Deprecating an entire module is somewhat easier problem than deprecating
an API that a module exposes. That said, I'll attempt to use the same tools
to find any usage of sys in the npm ecosystem:

  1. Run estoc against every version of every package in
    the npm ecosystem. This requires a fullfatdb clone of the registry.
  2. Grep estoc's output for "sys" usage.
  3. Sort usage by download count.

step 2: mitigation effectiveness

The question to be answered at this stage in deprecation is "how much effort will
it take to reduce breakage below a certain threshold?" We must take into account that
fixing the broken package itself does not guarantee that the number of downloads fixed
will go up. We're more concerned the the dependents of broken modules.

To estimate the amount of effort it will take to reach our target mitigation number,
we have to understand the network of dependencies around our target modules.

What we want to know is the minimum number of packages that will have to release a new patch
version before we reach the target mitigation number.

Given:

B           = the initial set of broken packages requiring a fix
Deps(p)     = the set of dependent edges on package@version "p", a pair of (package, range)
Fixes(r)    = whether a range r will pick up a patch version release – 0 if no, 1 if yes

We run the following pseudocode walk through the graph:

E = <empty set>
A = <empty set>
for package in B:
  A.add(B)

for package in A:
  for (dependent, range) in Deps(package):
    if Fixes(range) == 0:
      A.add(dependent)
    else:
      E.add(dependent)

The set, A, now contains all packages at versions that require PR fixes -
which will involve either bumping the patch version of an appropriate target
module, or removing the use of sys. Patching all packages in A would fix
100% of breakage, but would likely take too much time.

Now, given:

DLs(p)    = weekly number of downloads for package p
∂-(p)     = set of all dependents of package p
ownDLs(p) = DLs(p) - Σ(DLs(∂-(p))) – only the downloads originated by package p
P0 ➾͙ P1   = a potentially empty set of packages that represent the dependency chain
            between packages P0 and P1, non-inclusive
S(p)      = a tuple of (p, ownDLs(p)) for package p
Target    = target # of fixed weekly downloads

We run the following pseudocode:

list = []
for package in E ∪ A:
  list.push(S(package))

N = 0
fixablePackages = <empty set>
for package in sort(list, ownDownloadsDescending):
  N += ownDownloadsDescending
  for brokenPackage in B:
    fixablePackages = fixablePackages ∪ (package ➾͙  brokenPackage)
  if package in A:
    fixablePackages.add(package)
  if N >= Target:
    break

fixablePackages will now contain all of the packages necessary to fix the target mitigation
number of weekly downloads.

an aside

This is probably not the ideal way to collect this info! My maths are not the best.
Alternatively we could walk from each broken package along the dependent edge that introduces
the greatest number of weekly downloads, backtracking to the next largest source of weekly downloads
until we've met the target mitigation number.

Go / No go check: At this point there we should check to see if the amount of work
required to fully remove sys cleanly is achievable given the time frame.

step 3: go fix modules

Starting with the broken packages themselves, PR the appropriate fixes to packages. Expand to dependents as
new versions of broken modules are released. If a module is unmaintained, ask to take over.

Go / No go check: If v4.0.0 is looming at this point and we have not met our target mitigation number, the
we can't remove sys until v5.0.0.

@jasnell
Copy link
Member

jasnell commented May 15, 2015

@chrisdickinson ... posts like that should come with automatic and mandatory free beer to help mitigate the pain. Great analysis, going to have to stew over that one for a bit...

@rlidwka
Copy link
Contributor

rlidwka commented May 15, 2015

Starting with the broken packages themselves, PR the appropriate fixes to packages. Expand to dependents as new versions of broken modules are released. If a module is unmaintained, ask to take over.

What happens if a module FOO which half of npm depends upon outright refuses to make any changes, claiming that the deprecation which TC agreed upon is unnecessary, or that util is a name of a god in their native language they may not write in code, or whatever else reason?

And they are hoping that node.js will never actually change it because it'll break too much stuff, thus using their amount of downloads to indefinitely stall a deprecation of a certain feature?

Kinda similar thing happened with url module in 2.0.0 release, so I'd like to establish a policy on those cases.

@Fishrock123
Copy link
Contributor

What happens if a module FOO which half of npm depends upon outright refuses to make any changes

I'm not sure about the "outright refuses" part, but for modules where the maintainers no longer update them, we could always contact them / npm for access to publish a fixed version.

@rlidwka
Copy link
Contributor

rlidwka commented May 15, 2015

Yep, no longer maintained libraries are actually the simpler part. But actively maintained popular ones telling you "we don't want to change anything, our code works fine on 0.10.x and your new release is broken" is quite another matter.

Maybe I'll rephrase: what is the maximum period of deprecation after which the feature will be dropped no matter what?

@mikeal
Copy link
Contributor Author

mikeal commented May 15, 2015

If a good section of the ecosystem is objecting like that it's probably a good sign that we shouldn't move forward with the deprecation :(

@chrisdickinson
Copy link
Contributor

If a good section of the ecosystem is objecting like that it's probably a good sign that we shouldn't move forward with the deprecation :(

Agreed.

Maybe I'll rephrase: what is the maximum period of deprecation after which the feature will be dropped no matter what?

There is no maximum period of deprecation in that sense – we can only remove the feature once we reach the target mitigation number. If it becomes clear that we can't reach the target mitigation number, we should back out of the deprecation.

@yosuke-furukawa
Copy link
Member

deprecation has some stages.

for example:

  • soft deprecation (no warnings / no documents but TC decides to deprecate the API like cluster and util.is*)
  • document deprecation (no warnings but written document as deprecated API like fs.exists)
  • hard deprecation (print warnings like sys)
  • removal (the API is removed)

These stages are already documented ?
I think we should define the deprecation stage.

@brendanashworth
Copy link
Contributor

Perhaps after a deprecated feature is removed, an error is thrown with a link to something helpful (like a PR or reference issue) for fixing it.

@evanlucas
Copy link
Contributor

@chrisdickinson maybe this could help get started? https://github.com/evanlucas/find-sys/blob/master/results.txt. It doesn't include dependencies for all of the packages, only bundled dependencies, but it sure cuts the list down of what needs to be checked now

@geek
Copy link
Member

geek commented May 18, 2015

We need this to be simple and straightforward. What @yosuke-furukawa started is perfect.

What we don't need, is to be in the business of maintaining the world, @chrisdickinson.

@chrisdickinson
Copy link
Contributor

We need this to be simple and straightforward. What @yosuke-furukawa started is perfect.

What we don't need, is to be in the business of maintaining the world, @chrisdickinson.

@yosuke-furukawa's plan is necessary but not sufficient, and describes our current approach up until this point. However, we've never successfully removed code using that approach – we've deprecated, added messages, but have always stopped short of actually making breaking changes.

If we want to make breaking changes, we have to own up to it. We should have an idea of how much code we stand to break through removal, and we shouldn't remove APIs until the amount of code that will break is below a certain threshold. If there's something we really, really want to deprecate, then it's on us to make sure the ecosystem doesn't set on fire because of it – we have to meet the community at least half-way on this.

@geek
Copy link
Member

geek commented May 18, 2015

semver, documentation, time, and a clear policy should be more than sufficient. If you are concerned beyond that, then we can always release a non-core module that is for deprecated modules. In other words, take ownership of npmjs.com/package/deprecated or similar, and put the API that has been removed into that module as we remove it.

@chrisdickinson
Copy link
Contributor

semver, documentation, time, and a clear policy should be more than sufficient. If you are concerned beyond that, then we can always release a non-core module that is for deprecated modules. In other words, take ownership of npmjs.com/package/deprecated or similar, and put the API that has been removed into that module as we remove it.

This assumes a few things:

  1. We're always deprecating at module level, not individual API level.
  2. The module we're breaking doesn't create any transitive dependencies (that is, we don't create objects from that API that are then passed to other modules.)
  3. The breakage happens in code that most users can easily update.

In reality, a lot of packages with breakage may not be maintained, but are depended on indirectly by hundreds (or thousands) of packages. It's easy enough to change the packages that depend directly on the broken package, but now you've got to go one ring out and fix any package that pegged a dep on the packages you just fixed. It's a lot of work, potentially; and it has to be done before users can upgrade.

What the proposed changes state is that the users are the ones responsible for routing around whatever breakage we create – they have to shoulder the cost of fixing bugs and going through (essentially) the same process I've outlined above, but in an ad-hoc fashion with no central place for information sharing, and no option to "back out" of breaking changes that are too costly.

In reality, most folks won't upgrade – why would they? Of what benefit is a new Node sans sys that breaks their application? Only Node contributors (and, maybe, newcomers to Node) are getting any value out of that change – not at all the folks upgrading. The folks running their applications on Node hold all of the power: if they don't upgrade, the new version doesn't have any value. If we continue to keep them from upgrading with breaking changes, they'll switch platforms. If we want to break things, we've got to put in the legwork to make sure that we know what we're breaking, to mitigate as much of it as we can before releasing those changes, and to be able to back out of untenable breaking changes before we ruin a lot of folks' days by breaking their code.

In other words: we serve the users of Node, not the other way around. We can't dictate to them what Node will or won't be, their code already does that. We can't expect them to put in effort to change their code based on our say-so if we're not willing to step up and put our effort in as well.

@geek
Copy link
Member

geek commented May 18, 2015

@chrisdickinson I assume we are deprecating wherever we want to deprecate. If you deprecate some internal function then toss it into the deprecated module under the same path and parent file where its found.

What is your position ?

@chrisdickinson
Copy link
Contributor

A "deprecation" package on npm is not sufficient to fix breakage in the ecosystem, whether it's at module level or API level. Assuming such an approach could fix breakage, it still puts the onus on the end-user to fix the problem. They will still have to fix all of the modules that are broken between their code and the package that's actually broken, and point everything at the right versions. This is too much to demand of end-users.

@evanlucas: Great work! This should give us a good start.

@mikeal
Copy link
Contributor Author

mikeal commented May 18, 2015

Just a quick reminder that the long dep chains means incredibly long lists of potential packages that need to be updated because it's not enough to just push updates of those directly dependent, you must also get all of the deps of every other package to update to the new version, and so on, and so on, and so on :)

The good news is that there's a lot of data available about how many downloads of a particular package there are that we can use to assess the outstanding liability.

@chrisdickinson
Copy link
Contributor

Just a quick reminder that the long dep chains means incredibly long lists of potential packages that need to be updated because it's not enough to just push updates of those directly dependent, you must also get all of the deps of every other package to update to the new version, and so on, and so on, and so on :)

Exactly right! That's covered in the "mitigation effectiveness" step above – it's a bit subtle, but in the pseudocode walk through the graph we're looping over items of set A while adding packages to that set as we encounter them in that chain (halting at the first dep that would pick up a patch release of the next link in the chain.)

@Fishrock123
Copy link
Contributor

Probably another good candidate: #1747

@piscisaureus
Copy link
Contributor

Let's discuss (not necessarily decide) on tomorrow's TC meeting.

@JCMais
Copy link
Contributor

JCMais commented May 22, 2015

@piscisaureus Can we get an update on what was discussed?

@Fishrock123
Copy link
Contributor

@JCMais there wasn't a tc meeting this week, planning it fell through the cracks.

@orangemocha
Copy link
Contributor

I think documenting deprecation, including pointing people to the recommended alternative, plus allowing a good time window should be enough to deprecate functionality even if it's widely used. Number of major releases is not a good criteria, IMO, because those are driven by other factors like v8 upgrades and can happen frequently and not on a predictable timeline.

We might want to consider doing something to increase visibility of documented deprecation. I am worried that just because we give a "future deprecation" warning in the docs and release notes, people might not notice it immediately. In .NET, you can decorate types and methods with an "Obsolete" attribute, so that the compiler will give you a warning about it every time you compile. In Javascript there is no compilation, but perhaps we could achieve something similar by other means.

Just to illustrate the idea, a deprecated API could spew a one-time warning to stderr at runtime. Eg:
Warning: feature X will be deprecated by date Y, Consult link Z for more information. Use --deprecation-logfile to send this warning to a file instead of stderr.

@Fishrock123
Copy link
Contributor

Just to illustrate the idea, a deprecated API could spew a one-time warning to stderr at runtime

That is already what util.deprecate() does. :P

@orangemocha
Copy link
Contributor

😄

@rvagg
Copy link
Member

rvagg commented Jun 3, 2015

removed tsc-agenda label, please re-add if there's more to discuss here

@jasnell
Copy link
Member

jasnell commented Mar 9, 2016

@mikeal ... any reason to keep this one open?

@mikeal
Copy link
Contributor Author

mikeal commented Mar 9, 2016

Do we have a deprecation policy now? I couldn't find it earlier today.

@Fishrock123
Copy link
Contributor

the "official" deprecation policy is basically an evolved version of what is in this thread kinda. Dx

@mikeal
Copy link
Contributor Author

mikeal commented Mar 9, 2016

we should write something down :)

@Trott
Copy link
Member

Trott commented Aug 9, 2016

PR: #7964

@Trott
Copy link
Member

Trott commented Aug 9, 2016

I'm going to close this because the above-mentioned PR is where all the action is right now and if we want an issue to discuss it rather than a PR, that's #7912 rather than here (which has been inactive for 5 months).

If you feel strongly that this should remain open, by all means, re-open it.

@Trott Trott closed this as completed Aug 9, 2016
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

Successfully merging a pull request may close this issue.