docs(versioning): update guide with changes to event handlers #4688
Conversation
|
size-limit report 📦
|
|
|
||
| ### The element type in an event handler becomes broader | ||
|
|
||
| semver bump: **major** |
There was a problem hiding this comment.
Just to note that I am making this comment ActionList semantics issue in mind 🙂
It feels to me that types, especially the event type changes in that PR, feels a bit blur to me in terms of semver. Because on the consumers side, the event types are event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement> and since it doesn't overlap with event: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>, it fails. However if the types were less specific, or in other words, maybe not directly rely on the component details i.e. expecting the event to be triggered on the li element, that changes wouldn't break anything. Based on this, would this be more like "potentially breaking" ? 🤔 I would love to learn more about the best practises on the consumer side as well if you have thoughts on that! Is this a good practise to "hard code" the event types like currently or can they be inferred from what is defined in the component? 🤔
There was a problem hiding this comment.
However if the types were less specific, or in other words, maybe not directly rely on the component details i.e. expecting the event to be triggered on the li element, that changes wouldn't break anything. Based on this, would this be more like "potentially breaking" ? 🤔
I think you're spot on pointing out that it 100% depends on what the caller is doing with the types. As you pointed out, this is definitely a case where if the caller had a less strict type or used the type from the component prop interfaces there wouldn't be any breakage that we would observe.
In general, I think broadening a type could be considered "potentially breaking". I think we have scenarios where it is okay like in:
type ComponentProps = {
- variant?: 'a' | 'b',
+ variant?: 'a' | 'b' | 'c',
};But then we have cases like this with an event handler where it will cause issues downstream:
-onClick: (gesture: 'a' | 'b') => void,
+onClick: (gesture: 'a' | 'b' | 'c') => void,These seem like breaking changes to me since the caller cannot use the update without modifying their code. I think that the example for this with the generic would fall under this scenario since folks would have to update their code for it.
There was a problem hiding this comment.
Thanks Josh, this is great! I love the comparison you made above between the two examples.
These seem like breaking changes to me since the caller cannot use the update without modifying their code.
I like how you frame it. I think most of the time, this is a great indication to use to assess the versioning unless the consumer code is misused in the first place but this is another story 😅
Thanks for pushing this PR again, I appreciate talking about it and keeping it on the docs 🙏🏻
This came up in our release cycle this week and I wanted to document the behavior here to make sure we're aligned 👀
This updates our versioning guide with a specific case around making event handlers broaden, specifically when changing the element type on which the event occurs.
Changelog
New
Changed
Removed
Rollout strategy
This is a change to documentation