-
Notifications
You must be signed in to change notification settings - Fork 639
chore: Refactor stateful component check in constructState
#3251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
constructStateconstructState
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3251 +/- ##
==========================================
- Coverage 94.98% 94.98% -0.01%
==========================================
Files 511 511
Lines 11314 11300 -14
Branches 1754 1737 -17
==========================================
- Hits 10747 10733 -14
Misses 567 567 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| */ | ||
| export function isStatefulComponent(component: { type: string }): component is { | ||
| type: StatefulComponentType; | ||
| props: Record<string, any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ideal 🤔 Is this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we at least type it to be a union of the stateful component's props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought it was required at some point but it isn't anymore apparently, removed
| * @returns Whether the component is a stateful component. | ||
| */ | ||
| export function isStatefulComponent(component: { type: string }): component is { | ||
| type: StatefulComponentType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, does this then properly infer the props inside the if-clause in constructState? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have no issues in my IDE and when I look at the infered type its an union of the stateful components, I would like to send you a screenshot but I can't hit ctrl without the IDE context widow closing 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! 👍
This PR refactors
constructStateto use an util function calledisStatefulComponentthat uses an array of component types that includes all the stateful components.Fixes: #3235