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

Add more strict process around breaking changes #1918

Closed
reeganviljoen opened this issue Nov 27, 2023 · 8 comments
Closed

Add more strict process around breaking changes #1918

reeganviljoen opened this issue Nov 27, 2023 · 8 comments

Comments

@reeganviljoen
Copy link
Collaborator

Feature request

This is more of a process request

During my work with many colleges who use view_component the largest frustration that many developers I work with have is the constant breaking changes, many may decide to not use view_component on new projects, so I feel this is large enough issue

So my suggestion is allowing view_component to be backwards compatible to the last major version and use deprecation notices for a full major version to allow the ability to migrate over a longer period of time

Also all breaking changes should be documented more explicitly and possibly for breaking changes to be introduced a migration guide must be in place as a criteria

I would like to close this off as saying this is all a suggestion and not an ultimatum, I just wanted to give a few suggestions and bestow how dire this situation can get from my interactions with real users of this library

@reeganviljoen
Copy link
Collaborator Author

@Spone @BlakeWilliams @boardfish(apologies to anyone I left out) can you please take a look at this and leave your thoughts

@boardfish
Copy link
Collaborator

So my suggestion is allowing view_component to be backwards compatible to the last major version and use deprecation notices for a full major version to allow the ability to migrate over a longer period of time

If I'm understanding you correctly here, is this to say that if we wanted the next version to introduce breaking functionality, we should wait two major versions to remove the old functionality entirely:

graph LR
    A[V1] -- Functionality introduced with backwards compatibility and deprecation warnings --> B
    B[V2] -- Backwards compatibility removed --> C[V3]
Loading

...as opposed to the current process, which is:

graph LR
    A[V1] -- Functionality introduced with backwards compatibility and deprecation warnings. Backwards compatibility removed --> B
    B[V2]
Loading

@reeganviljoen
Copy link
Collaborator Author

@boardfish I feel that may be a good idea, I myself only started using view_component in 3.0 and I love it but manny users that have been using view_component 2.0 have not had the best time moving to version 3 and have been actively soured on it, I worry about this as it seems to be a large opinion in the circles I work in and this could ultimately make less people want to use view_component in all its awesomeness

@boardfish
Copy link
Collaborator

boardfish commented Nov 28, 2023

I think it's valid to say that the migration path to V3 might've felt rocky for most, especially at a larger scale, primarily due to the change from slot_name to with_slot_name. At @fac, we needed to upgrade to the next major version both on the main app and within an internal gem dependency. The scale of the work and inherent risk that came with that meant that we didn't upgrade until fairly recently, so we were still on a late V2 minor version for a good while.

The aforementioned change was strictly a rename. If we had full confidence, in, say, a RuboCop rule, or better yet, something that could autoload all components and determine which slots had been defined, then use that to autocorrect, it might've been much safer and we could've felt more confident about changing everything in one fell swoop. To @Spone's credit, that's been in the works in #1669, but perhaps that piece of work should have been a prerequisite for the version bump as a whole.

While I do agree that we could've made the migration path for this change much easier, I worry that removing breaking changes across two major versions could cause its own difficulty, as it forces us to leave old code in the codebase for much longer.

There are probably processes that we could introduce to support this, such as by migrating deprecated code to "backport" modules that, when included, override and/or undefine methods to restore things back to the way they were. Doing so would mean that the major version bump would constitute deleting those files. However, I think that introduces a lot of risk (that we can't fully replicate the old state) and overhead (in moving the code out to these files and patching classes to make sure they're working under the exact same conditions).

Maybe this is a signal that we could do better at signalling deprecations – have we put those in release notes in the past?

@boardfish
Copy link
Collaborator

I do also want to shout out deprecation_toolkit – we found this really useful in our V3 migration.

@camertron
Copy link
Contributor

camertron commented Nov 29, 2023

Hey @reeganviljoen, thanks for bringing this up!

the largest frustration that many developers I work with have is the constant breaking changes

I'm curious specifically about your use of the word "constant" here, since we try pretty hard to follow semantic versioning and only introduce breaking changes in major versions. I'd love to hear more about which changes in particular people are finding difficult, because my intuition is there are unique reasons why each individual change is challenging. For example, @boardfish mentioned renaming slot methods. That migration was challenging for us at GitHub too, mostly because Ruby doesn't have a great way to identify methods called on component instances (aside from grep). In the slot renaming case, we printed deprecation warnings and provided a clear upgrade path, waiting until v3 to switch over to the new naming strategy. What other upgrade issues are people running into?

my suggestion is allowing view_component to be backwards compatible to the last major version and use deprecation notices for a full major version to allow the ability to migrate over a longer period of time

I think this describes our existing process. Minor/patch releases of ViewComponent are backwards-compatible with the last major version... or at least, they should be. Again, I would be genuinely curious to hear about specific upgrade issues. We should not be making backwards-incompatible changes in major/patch releases, and if we are, we should stop.

Also all breaking changes should be documented more explicitly

Ok that makes sense to me. We probably shouldn't rely exclusively on deprecation warnings. I have a few ideas here:

  1. Format the CHANGELOG to indicate which changes are breaking vs non-breaking. Here's an example from the Zeitwerk gem.
  2. In the same vein, use changesets to indicate which changes resulted in a major/minor/patch version bump. See this example from primer/view_components.
  3. Maybe mention breaking changes on the front page of viewcomponent.org?
  4. Other stragegies?

...a migration guide must be in place as a criteria

Yes, this I agree with 100%. We should have had a migration guide for the jump from v2 to v3. However, I do question how useful a migration guide would have been for migrating to the new slot names API, since there's nothing anyone could have written that would have made it less painful 😅

@reeganviljoen
Copy link
Collaborator Author

I'm curious specifically about your use of the word "constant" here, since we try pretty hard to follow semantic versioning and only introduce breaking changes in major versions. I'd love to hear more about which changes in particular people are finding difficult, because my intuition is there are unique reasons why each individual change is challenging. For example, @boardfish mentioned renaming slot methods. That migration was challenging for us at GitHub too, mostly because Ruby doesn't have a great way to identify methods called on component instances (aside from grep). In the slot renaming case, we printed deprecation warnings and provided a clear upgrade path, waiting until v3 to switch over to the new naming strategy. What other upgrade issues are people running into?

@camertron the largest frustration I have seen is indeed slots, and an example that was given to me is if you look at rails active record, it really hasn't had many breaking changes between major versions, even action_view itself has soft deprecated form_for for many versions now but it is still available.So what I suggest is we try to soft deprecate things where possible(I understand slots was a painful jump, and is the exception) and only remove things where absolutely necessary

I think this describes our existing process. Minor/patch releases of ViewComponent are backwards-compatible with the last major version... or at least, they should be. Again, I would be genuinely curious to hear about specific upgrade issues. We should not be making backwards-incompatible changes in major/patch releases, and if we are, we should stop.

The largest breaking change between major versions that causes frustration is slots between v2 and v3, and again I understand it was necessary, It just may be wise to approach it more carefully next time with a migration guide and so forth

Ok that makes sense to me. We probably shouldn't rely exclusively on deprecation warnings. I have a few ideas here:

  1. Format the CHANGELOG to indicate which changes are breaking vs non-breaking. Here's an example from the Zeitwerk gem.
  2. In the same vein, use changesets to indicate which changes resulted in a major/minor/patch version bump. See this example from primer/view_components.
  3. Maybe mention breaking changes on the front page of viewcomponent.org?
  4. Other stragegies?

...a migration guide must be in place as a criteria

Yes, this I agree with 100%. We should have had a migration guide for the jump from v2 to v3. However, I do question how useful a migration guide would have been for migrating to the new slot names API, since there's nothing anyone could have written that would have made it less painful 😅

I agree with all the suggestions, a migration guide would have still been helpful as deprecation warnings are easier to get muddled up between other deprecation warnings of other gems

@camertron
Copy link
Contributor

Alright thanks for your feedback @reeganviljoen, we'll take it into consideration for the next major release 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants