Doist / reactist

Open source React components made with ❤️ by Doist
http://doist.github.io/reactist
MIT License
247 stars 21 forks source link

fix: Omit props removed in future versions of @types/react #819

Closed pawelgrimm closed 4 months ago

pawelgrimm commented 4 months ago

Short description

In newer versions of the @types/react package, certain props on certain components have been removed. When consumers of Reactist upgrade that package, they will run into type errors stating that there are missing props on a bunch of Reactist components. For example, the Button component would require the placeholder, onPointerEnterCapture, and onPointerLeaveCapture props to be specified on all usages of that component.

Eventually, we should upgrade the @types/react package in this repository as well, but this is blocked by upgrading to the latest @ariakit/react version. In the interim, the fix in this PR should allow consumers to continue using this package without issues.

PR Checklist

Versioning

Breaking change (major)

gnapse commented 4 months ago

Should this PR be made on top of next instead? Or at the very least, do it there as well? That's the branch that we're currently consuming in todoist-web.

pawelgrimm commented 4 months ago

Should this PR be made on top of next instead? Or at the very least, do it there as well? That's the branch that we're currently consuming in todoist-web.

@gnapse That's a great question. What's your plan for releasing the current beta version (v24.1.2-beta) of Reactist? Should I wait for that to happen before I cut this into a patch release? Or should I merge this in and release it as v24.1.3-beta?

gnapse commented 4 months ago

What's your plan for releasing the current beta version (v24.1.2-beta) of Reactist?

I first want to address the issue about the as="…" prop. And unfortunately, I haven't been able to make time for it this week.

Should I wait for that to happen before I cut this into a patch release? Or should I merge this in and release it as v24.1.3-beta?

No, let's not wait. Let's put this in a new beta release.

pawelgrimm commented 4 months ago

@gnapse I'm a bit torn on the type of version bump to use here. Technically, it's an API change, since we're changing the props we accept for a bunch of components. On the other hand, this is purely a type-level change and we're still passing those props forward, though it's unclear if those props were ever actually supported. Do you agree that it should still be a new major version though?

gnapse commented 4 months ago

I'm as torn as you, but with the recommendation to add it to the beta v24 (without increasing it to v25). v24 is set out to be the next major release (once we get past the issue with the as="…" prop). So it is the perfect opportunity to release this change, also as part of the breaking changes that prompt the jump from v23 to v24. Makes sense?