[Button] Fix button types #4111
Conversation
|
👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. |
|
Hi. Have an old issue: #521 In particular note @lemonmade's comment that points out that while this is good for use on web it is fundamentally incompatible in other worlds like Argo and exposing these dom events, and potentially relying on them is going to be a big problem for Argo. |
|
Hey @BPScott, I think it'd be worthwhile to re-visit this discussion. @lemonmade's comments around how this is fundamentally incompatible with Argo make sense and I generally share those concerns. I also agree we should be discouraging consumers from relying on these events and opening issues instead for the things that are currently hard to implement. With that said, I fundamentally disagree with the current strategy of type obfuscation. It's one thing not to document the fact that events are available in order to discourage use; on the other hand, type obfuscation has real consequences with regards to the type safety of consuming applications. This recently caused a bug in production for us on the Online Store because we passed a handler that accepted optional arguments to a function foo(optionalArg?: boolean) {
if (optionalArg) {
throw new Error();
}
}
<Button onClick={foo} /> // Typed as () => void, but is in fact (event: React.SyntheticEvent) => void
// On click, error is thrown because `foo` is called with `event`Were these handlers properly typed, TypeScript would have caught this issue. If the event is there, the types should reflect this, regardless of whether or not we want to discourage use. |
Happy to rediscuss and provide context, though I'm not on the polaris team these days so the final decision will fall to @Shopify/polaris-team. :) |
|
@alex-page not sure if you had a particular thing in mind from my review, but my opinion on this is pretty much unchanged. I agree with @clauderic that the current lie the API tells you needs to be resolved, but I would resolve it by changing the implementation of |
alex-page
force-pushed
the
main
branch
7 times, most recently
from
2c6e842 to
d2611d6
Compare
5 participants
WHY are these changes introduced?
I'm working on a task in
Shopify/webwhere I call the preventDefault function (event handler), but it's not returning the appropriate types for Button callbacks.WHAT is this pull request doing?
Fixing button types (callbacks)
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes