ionic-framework icon indicating copy to clipboard operation
ionic-framework copied to clipboard

feat: Improved types for react useIonModal

Open aeharding opened this issue 2 years ago • 8 comments

Prerequisites

Describe the Feature Request

useIonModal should type props parameter.

Describe the Use Case

useIonModal currently allows any for component props. This is dangerous and has caused many crashes and unintended behaviors in my experience.

Currently:

export declare function useIonModal(component: ReactComponentOrElement, componentProps?: any): UseIonModalResult;

Describe Preferred Solution

export declare function useIonModal<P>(component: React.ComponentClass<P> | React.FC<P>, componentProps?: P): UseIonModalResult;
export declare function useIonModal(component: React.ReactElement, componentProps?: any): UseIonModalResult;

Describe Alternatives

Leaving as-is

Related Code

No response

Additional Information

If you would accept this change, I would happily make a PR

aeharding avatar Dec 11 '23 04:12 aeharding

FYI I see #27548, this seemed a bit more open-ended so I decided to make a separate issue in smaller scope (fixing the types).

aeharding avatar Dec 11 '23 04:12 aeharding

Thanks! I think this would be a good change to make. We'll want to do make this change for both useIonModal and useIonPopover, and T should continue to be any if not set to avoid breaking changes.

liamdebeasi avatar Dec 11 '23 14:12 liamdebeasi

@liamdebeasi, in my testing Typescript automatically infers T based on the component you pass it. By making the proposed change I was able to see type errors with incorrect params in my usage without adding any explicit generic declarations to my code :)

aeharding avatar Dec 11 '23 14:12 aeharding

in my testing Typescript automatically infers T based on the component you pass it.

I'm aware, I was just clarifying that we shouldn't default it to anything else 😄

liamdebeasi avatar Dec 11 '23 14:12 liamdebeasi

This issue has been labeled as help wanted. This label is added to issues that we believe would be good for contributors.

If you'd like to work on this issue, please comment here letting us know that you would like to submit a pull request for it. This helps us to keep track of the pull request and make sure there isn't duplicated effort.

For a guide on how to create a pull request and test this project locally to see your changes, see our contributing documentation.

Thank you!

ionitron-bot[bot] avatar Jan 09 '24 22:01 ionitron-bot[bot]

Is this still relevant?

mikelhamer avatar Jul 23 '24 17:07 mikelhamer

Yes, and my PR was close to being merged, but @liamdebeasi left Ionic so no progress since

aeharding avatar Jul 23 '24 17:07 aeharding

Ah, thank you. I'm new here and digging around for ways to help. So your PR partially completed the feature, but there is some left to do?

mikelhamer avatar Jul 23 '24 17:07 mikelhamer