Shopify / deprecation_toolkit

⚒Eliminate deprecations from your codebase ⚒
MIT License
460 stars 40 forks source link

Throwing errors when dependencies are using deprecated methods #44

Closed mattdrose closed 4 years ago

mattdrose commented 4 years ago

Behaviour

DeprecationToolkit is throwing errors if a consumer is using a deprecated method. For example, if FooApp is using Rails ~6.0, and a dependency is using Module#parent, FooApp will throw an error.

Expected behaviour

DeprecationToolkit only throws an error if there's a deprecated method used in the source code. For example, FooApp shouldn't throw and error if a dependency is using Module#parent, but rather only if Module#parent is being used in the source code.

What people are doing to solve it

I see apps like core are ignoring these warnings.

Opinion

I'm not sure if this is by design or not, but if it is I don't think it should be default behaviour.

Shitlists are a great pattern to update your codebase—but it's not the task of an app to also update their dependencies codebases as well. For example, if Rails deprecates a method, it should be up to the app maintainers to update their codebase, and separately up to the gem maintainers to update their own codebase. In this case, each maintainer would create a shitlist for their respected codebases.

rafaelfranca commented 4 years ago

Thank you for the issue. It is by design. We believe application developers should be also responsible for the libraries they use in their apps, after all, they chose to use those dependencies and are responsible for making sure that choice is still the right one.

If a library in their app is using deprecated code either the app developer should open a PR to the library, or they should report an issue. In case it is already fixed this failure is also useful to get the app developers to upgrade the dependencies.

We are aware that some cases it is not possible to do those things, like in the case you pointed. But that is likely because the dependency is not maintained anymore or because it is an old version. In those cases either the app need to remove that dependency in favor of something that is maintained, or they need to upgrade. It may take time and that is is why we are ignoring that warning in the cause you linked.

rafaelfranca commented 4 years ago

Oops, I'm was not supposed to close this

mattdrose commented 4 years ago

Thanks for the quick reply. Knowing it was design, this is a discussion rather than a bug.

Your reasoning seems like good practice, and I agree with the reasoning. In practice though, there are a couple caveats I've noticed:

  1. As you mentioned, this requires consumers to ignore warnings in some cases.
  2. You end up with multiple shitlists tracking the same thing (both consumers and libraries will have duplicate recordings)
  3. Although consuming apps have the benefit of locking their app to singular versions, libraries don't always have the same benefit. This means that libraries need to either refactor code to avoid deprecations as well as support existing environments, or drop a supported environment before they would otherwise have to. In the former case, the library is then forced to create a new shitlist in order to keep track of all the places they had to develop work arounds in order to avoid deprecations. I believe libraries should have the freedom to decide the best timing to remove deprecations according to the circumstance in order to support the environments they've decided to support. All this, without forcing consumers to swallow recordings or ignore warnings.

What are your thoughts on making this behaviour opt-in or opt-out? Or even possibly detecting whether a library is already maintaining their own shitlist, and not throwing errors if this is true.

rafaelfranca commented 4 years ago

This gem doesn't target libraries, only apps. So we would not have shitlists inside the libraries.

As the deprecation_toollkit only run in the test environment, even if a library had a shitlist, users would still see deprecations in production and development, and libraries using deprecated code would still have to change their code, otherwise they would be annoying the users that would likely open issues to them fix the deprecations.

This is what already happens today, and the beginning of the Ruby community, and in my opinion is one of the best things of the Ruby ecosystem, we evolve fast and adopt early.

If a consequence of this gem existing is that libraries keep using deprecated code I'd probably unpublish this library 😄.

All that said, the goal of this gem is make easier for application developers know which code they use in their application will be broken when the deprecated code is removed from the source, and progressively fix this code. This includes code on libraries and in the application. Ignoring deprecated code on libraries would just make this future breakage invisible to the app developers, what would defeat the reason why we wrote this gem.

mattdrose commented 4 years ago

Closing this since the gem is doing its job, and I don't have anything left to say :P. Thanks for hearing me out and walking me through your reasoning 🙏.