Skip to content

docs(versioning): update guide with changes to event handlers#4688

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.

Sign up for GitHub

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

Merged
joshblack merged 1 commit into from
Jun 25, 2024
Merged

docs(versioning): update guide with changes to event handlers #4688

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions contributor-docs/versioning.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -16,6 +16,7 @@
- [A component includes a landmark role](#a-component-includes-a-landmark-role)
- [A component no longer includes a landmark role](#a-component-no-longer-includes-a-landmark-role)
- [The element onto which props are spread is changed](#the-element-onto-which-props-are-spread-is-changed)
- [The element type in an event handler becomes broader](#the-element-type-in-an-event-handler-becomes-broader)

<!-- END doctoc generated TOC please keep comment here to allow auto update -->
<!-- prettier-ignore-end -->
Expand DownExpand Up@@ -56,6 +57,7 @@ For a full list of releases, visit our [releases](https://github.com/primer/reac
| | A prop is deprecated | `minor` |
| | A prop is removed | `major` |
| | [The element onto which props are spread is changed](#the-element-onto-which-props-are-spread-is-changed) | potentially `major` |
| | [The element type in an event handler becomes broader](#the-element-type-in-an-event-handler-becomes-broader) | `major` |
| Package | A dependency is added | `minor` |
| | A dependency is removed and it does not affect the public API of the package | `minor` |
| | A dependency is removed and it does affect the public API of the package | `major` |
Expand DownExpand Up@@ -246,3 +248,65 @@ function Component(props: Props) {
```

</details>

### The element type in an event handler becomes broader

semver bump: **major**

@broccolinisoupbroccolinisoupJun 24, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

@joshblackjoshblackJun 24, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@broccolinisoupbroccolinisoupJun 25, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 is a breaking change due to how extracted event handlers need to be typed.
Consider the following example:

```tsx
function ExampleComponent() {
return (
<Item
onSelect={event => {
//
}}
/>
)
}
```

When the `onSelect` handler is provided inline, the type is inferred and does
not need to be explicitly defined. However, when this handler is pulled out then
the type is defined:

```tsx
function ExampleComponent() {
function onSelect(event: React.MouseEvent<HTMLDivElement>) {
//
}

return <Item onSelect={onSelect} />
}
```

This situation is what will cause a breaking change if we change the base
element type of this event handler. For example, if we update the props of
`Item` to be the following:

```diff
interface ItemProps {
- onSelect(event: React.MouseEvent<HTMLDivElement>): void;
+ onSelect(event: React.MouseEvent<HTMLElement>): void;
}
```

Then the downstream consumers that have typed `onSelect` will now have a
TypeScript error since `HTMLDivElement` does not fully overlap with `HTMLElement`.

When choosing to expose event handler types, consider what the minimum type is
that will satisfy the requirements by callers. Additionally, when typing
handlers of components it is recommended to use the type from the component
props directly. For example:

```tsx
function ExampleComponent() {
const onSelect: ItemProps['onSelect'] = event => {
//
}

return <Item onSelect={onSelect} />
}
```