[WIP] fix: react types for show() no longer make every prop optional

[geowarin-sg]

Using Partial<Props> makes every component props optional. This was probably done in #9 because create() used to take Record<string, unknow> as a type parameter which did not provide type safety.

By using P coming from React.ComponentType<P>, we are able to fix this.

[supnate]

LGTM Thanks @geowarin-sg ! @xxleyi Would you please also take a look at this change?

[xxleyi]

@supnate @geowarin-sg As my understanding of nice-modal-react, we have serval ways pass props to Modal component. So even we have some required props, when we use one of api, such as show, we don't have to pass those required props, which may have be passed by useModal or register.

[geowarin-sg]

If you create a Modal component that has required props, you want those to be required by show(). Other optional props coming from the nice-modal API are still optional.

Maybe I'm missing something here but that how I would like to use it 😄

[supnate]

Thanks @xxleyi for pointing this out.

@geowarin-sg The useModal API also allows to pass props to the component. Then it doesn't need to pass args to show method, for example:

const modal = useModal(MyDialog, { name: "Nate" }); // MyDialog requires name prop
//...
modal.show(); // no need to pass name prop again

So I think partial is reasonable. We would just want the API to be easy to use since you may call modal.show in different places with one initial config in useModal. It at least provides type safety by avoiding wrong props being passed.

[geowarin-sg]

Gotcha! So this PR won't do. I can do a new one where the parameters you pass to useModal() are substracted from the one you need to pass to show().

[jakst]

I experimented a bit with a type that would be flexible enough for this, and came up with something like this (doesn't include React component type atm):

https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABCAzgUwLJwCYEMA2APAFCKIDCcAtgA4JphQAKATnDSgDSKmKto1cLNNlbsUiNAA8oDbBKZDYBQpVr1GYjgD5uvAEpoquGGFMBzLRIC8iAPJUYUVdTpgGzNh24BrNAE84YD5hQWFRLxRtRAAyPiUYFTU3DyttYm0ACghXDSgALkRMmkjC5LyrAEpEa2jcMH9uEoEhEStC-jC2yMrCgG9eFAALOAB3QsNjUwsrSRk5CUMARxAYcMJJkzMwS0jogH4eMmOi4Snt3fEJoy2ZnprogDc4GGxeMkLMs9udq33r853cTVWqIZ6vYgAX0QAzIwigIBYSD60NwEnq-ihxGIOTAKCgiAw-gAIol8HBzDUiiUrjDEGBcFQ0IV8SwLNxcOZmfSQFQAEZoFjcfBCLn-RB8uBwfBoeqISEg6J9eXY3H4xBUHAEADqTiGGBgKBQFmWq3CWipqEwWvwmSJpIIFO4yoZTMKACIAHK4WTu+WVbEAekD9jA+H8iFw2GwFkQUCGaEQwhWaxE3D5IAJ4GaXWwiBpHEQwzgIHwedGcBYPmImrw+F18YNRpNaBT5q8ADpi6NMsrOdyAIwAJn9QZD2oTSCjMZ2cYTyDAOdaeeTZpE+a83ECIEQEDlBBQcEQcEegrZ2ETBZQNZtDf1huNO1NqYi7C7Ix7fa5hWH3Fd3K9H00D9BUbzrO8m0fcxn3bN9u17SNv0QX9EBFFgkKgFgQETUDax1PVIJbNtujgj8EP7H8h2FUVuUw7C-0ZADvV9UdiGDRBPTgAlp1jeNE2zUJlyTVs1zzK8ixGUs82AEx8DA-DGwfIjRK0d8xl7XDbwIpSnxEl9VPgl1GI9ZjgNHPD6205tdOI18aDUz9UJowo6JwgMLIgnToL02D7MM+ljMQQDfWo9DaKwtzsRxBB1Q8vUAEF8HwGCSI4TohNsK0sDrO0STJJ06X-EygPdDkkOHVj2InBhIyS4TbI3cRI2EWr8GEKMIyXcJuD4xrCzgPkACs0GgRBDWPGhYAQAh5Ms+NEuSnzUpQdLwgczJ3K0+akpSuyVsEtbDKKoLTPdUCoqioA

[jakst]

Feel free to take over/use/modify ☝️, I'm not actively using this library at the moment so I won't open a PR myself

[supnate]

Thank you @Jakst ! So @geowarin-sg or @xxleyi would you please help to create the PR? To be honest, this is my first Typescript project, is being surprised about the flexibility of the type system. Thanks for you guys' help on it!

[xxleyi]

@supnate @geowarin-sg I re-read the API of NiceModal. I found they are very flexible, which may stop us use more strict TS type.

we have register method on NiceModal, which can also pass props to Modal component. Usually we use pair of NiceModal.register and NiceModal.show, or pair of const modal = useModal... and modal.show, but it seems nothing will stop us to mix NiceModal.register, const modal = useModal... and modal.show, in which case, TS generics will not work in my perspective.

[supnate]

Thanks @xxleyi for the deep dive. For register or <MyModal id='myid' .../> to define a modal. You need to use useModal('modalId') by string id to get the modal handler. So they should not affect the type definition for useModal(ModalComp). So it should be possible as @Jakst and @geowarin-sg suggested.

[geowarin-sg]

I will try and base this PR on @Jakst example. I'll make sure useModal('id') is unaffected. I haven't got a lot of time on my hands right now. Maybe next week but no promises 😄

[supnate]

Thanks @geowarin-sg ! Take your time.

[supnate]

Accidentally merged... Reverted by PR #29 .