In v31 there have been a couple of violations of semver when minor was bumped instead of major.
The two things that impacted me were:
31.4.0which introduced grid virtualisation which changes the default behaviour for a grid's height, i.e. can no longer be a direct child of a flexitem.31.6.0which deprecated usingEuiOverlayMaskwhen usingEuiModaland removed it'sonClick.
And looking at the changelog, I see 31.5.0 did the same forEuiConfirmModal.
I realise semver's definition of 'breaking changes' can be somewhat subjective, but I'd say anything that isn't a patch but changes default behaviour or deprecates something, should definitely bump major.
I'll be honest, I wouldn't normally raise an issue for this, but EUI is such a big part of my project that I need to have confidence that a minor bump will only introduce backwards compatible changes.
For now, I'm depend on exactly 31.0.0, rather than ^31.0.0.
j-m
All 7 comments
Thank you for mentioning both of these, and I apologize for their impact on your project. In retrospect, the datagrid's DOM interaction probably should have stood out more, as we had specific instances in testing where layouts needed to account for it. This one in particular is right on our line when considering something as a breaking change or not - on one hand we cannot try to account for every possible layout, but this certainly affected known implementations.
The modal/mask change did remove functionality, and even if an implementation kept its EuiOverlayMask that one is covered up by EuiModal->EuiOverlayMask and so the click handler is never fired. I think this was an oversight on our part, and I'll check if we want to return to supporting an onClick on the overlay, provided through the modal's props.
I'm adding this to next week's team discussion and will reply with any additional info we come away with.
chandlerprall
on 26 Feb 2021
Just to add a thought to the broader discussion of semver (though not directly these issues): it's struck me before how we're a little loose with deprecations.
Sometimes we just remove stuff. Other times, we label something as deprecated in the docs but don't consistently warn in code (and then remove it ~6 months later). But there doesn't seem to be any rhyme or reason on when each case happens - just someone decides if it's "big" or not but that seems _really_ subjective.
myasonik
on 26 Feb 2021
No problem! I'm just happy that Elastic is so open to community feedback, it's quite rare and definitely valued, so thank you!
I would strongly recommend automatically versioning and releasing with _every_ merge.
With a project this big you'll likely end up with minor versions in the hundreds or thousands, but this is desirable; you want the granularity.
j-m
on 27 Feb 2021
Quick follow-up & closing, but feel free to re-open if you'd like to continue a discussion on any of these.
Removal of onClick support from the mask is intentional and will not be supported in the new configuration. We consider dismissal through clicking on the mask as too ambiguous of an operation from both user & app perspectives - e.g. does it apply or throw away changes - which are better communicated through the Close button or other elementsI within the modal.
it's struck me before how we're a little loose with deprecations.
We will keep an eye on deprecated-first vs. non-deprecated breaking changes as they arise. Initially, I'd prefer deprecations as the default but I'm not sure how often that is viable. Looking back at the breaking changes we've had is likely a good first step.
I would strongly recommend automatically versioning and releasing with every merge.
We did explore automatic versioning a while back and decided that the trade offs to our workflow weren't worth it. There is more automation we'd like to see happen, particularly around changelog entries, but for now we feel there's more benefit from manually coordinating releases.
chandlerprall
on 2 Mar 2021
What's changed as a result of this ticket?
If not automatic versioning then maybe your release script could look for keywords in commit messages?
May add extra checkbox questions in PRs? "Did you remove any props? Did you remove any components? Did you virtualise datagrid? ;)"
j-m
on 3 Mar 2021
Hey @j-m , We actually discussed this ticket as a whole team yesterday. It was first on our agenda. In the end, we did agree that we might have slipped up in the past few releases with being too lenient on deciding what's a breaking change. However, our current workflow works really well for us and see more pitfalls than positives when it comes to automatic releases.
What's changed is that we have all agree to be more diligent and mindful of what we consider breaking vs non-breaking changes. We already have a checklist item for 'Is this a breaking change?' that will serve as a reminder.
We're sorry that the last few releases inadvertently caused breaks for you and we will do better in the future.
cchaos
on 3 Mar 2021
Okay, sounds good!
Thanks all 馃挏
j-m
on 3 Mar 2021
Related issues
jen-huang
路
4Comments
stacey-gammon
路
4Comments
igoristic
路
4Comments
thompsongl
路
3Comments
j-m
路
3Comments
Most helpful comment
Just to add a thought to the broader discussion of semver (though not directly these issues): it's struck me before how we're a little loose with deprecations.
Sometimes we just remove stuff. Other times, we label something as deprecated in the docs but don't consistently warn in code (and then remove it ~6 months later). But there doesn't seem to be any rhyme or reason on when each case happens - just someone decides if it's "big" or not but that seems _really_ subjective.