feat(react): type overlay hook component props - #31362
Open
ShaneK wants to merge 1 commit into
Open
Conversation
Co-authored-by: Alexander Harding <2166114+aeharding@users.noreply.github.com> Co-authored-by: Liam DeBeasi <2721089+liamdebeasi@users.noreply.github.com> Co-authored-by: Ren yiwei <85666259+SSDWGG@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue number: resolves #28680
What is the current behavior?
Currently,
useIonModalanduseIonPopovertakecomponentPropsasany, so nothing checks the props against the component being presented. Typos and missing props show up at runtime instead of at build time.This has been attempted three times. #28685 and #29058 stalled in review, and #29158 was auto-closed when the
feature-8.0branch was deleted rather than rejected. All three were deferred on the grounds that the added strictness needed a major, which is what this branch is. The most recent community attempt, #31183, leavescomponentPropsoptional, which brings back the gap that earlier review had already settled: a component with required props still type checks when you pass no props at all.What is the new behavior?
Both hooks now infer
Propsfrom the component and checkcomponentPropsagainst it, andcomponentPropsis required when the component declares required props. Passing a JSX element is unchanged and stays permissive.Propscomes from the component alone, usingNoInferoncomponentProps. That part isn't cosmetic. The overlay docs passdismissback in throughcomponentProps, and inferring from that argument needs the type ofdismisswhile the same statement is still declaring it, which TypeScript rejects as circular. The cost is that a component declared inline needs its props annotated.NoInferlands in the published types, so@ionic/reactnow needs TypeScript 5.4.@ionic/angularalready required 5.4, so this lines them up, and a newreact-typescriptmigration raises the pin for React apps the same wayangular-typescriptdoes.Writing this turned up two existing mistakes that the old
anywas hiding: the root modal in the React test app was rendering withtypeandcountundefined, and the v9 modal controller docs example didn't type check. Both are fixed.Does this introduce a breaking change?
Apps passing props that don't match their component will get new compile errors, which is the point of the change. The fix is to correct the call sites. Apps below TypeScript 5.4 need to upgrade, and
npx @ionic/migratehandles the pin. Notes are in BREAKING.md and the v9 upgrade guide.Other information
The overload shape comes from Liam's #29158, the requirement that
componentPropscan't be omitted comes from Alexander's review on #29058, and the compile-time spec pattern comes from Ren yiwei's #31183. All three are credited as co-authors on the commit.Nothing to preview. The change is types only at runtime.
Also, I'm aware this is way too close to the release of v9. This may need to be held off until v10, which won't be too long after v9 comes out.