-
Notifications
You must be signed in to change notification settings - Fork 443
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
Comments
@Spone @BlakeWilliams @boardfish(apologies to anyone I left out) can you please take a look at this and leave your thoughts |
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]
...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]
|
@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 |
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 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? |
I do also want to shout out deprecation_toolkit – we found this really useful in our V3 migration. |
Hey @reeganviljoen, thanks for bringing this up!
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
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.
Ok that makes sense to me. We probably shouldn't rely exclusively on deprecation warnings. I have a few ideas here:
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 😅 |
@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
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
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 |
Alright thanks for your feedback @reeganviljoen, we'll take it into consideration for the next major release 😄 |
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
The text was updated successfully, but these errors were encountered: