-
Notifications
You must be signed in to change notification settings - Fork 58
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
AF-2922: add defaultProps getter to UiProp #196
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on HipChat: InfoSec Forum. |
+1 Might be good to do a quick check to make sure no one's declaring a |
Codecov Report
@@ Coverage Diff @@
## master #196 +/- ##
==========================================
+ Coverage 94.27% 94.55% +0.29%
==========================================
Files 34 34
Lines 1640 1651 +11
==========================================
+ Hits 1546 1561 +15
+ Misses 94 90 -4 |
Looks like |
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.
+1
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.
Just noticed two small things
@@ -690,6 +690,11 @@ abstract class UiProps extends MapBase | |||
} | |||
|
|||
Function get componentFactory; | |||
|
|||
Map get componentDefaultProps => componentFactory is ReactDartComponentFactoryProxy |
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 should have a doc comment
Map get componentDefaultProps => componentFactory is ReactDartComponentFactoryProxy | ||
// ignore: avoid_as | ||
? (componentFactory as ReactDartComponentFactoryProxy).defaultProps | ||
: const {}; |
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 add a test case for this case?
e.g.,
expect(Dom.div().componentDefaultProps, equals({}));
QA +1
Merging into master. |
Ultimate problem:
With the new builder, the implementation of
typedPropsFactory()
will no longer be available on the component class defined by the consumer – it will only be available on the generated component class. As a consequence, all instances ofnew FooComponent().getDefaultProps()
will break.How it was fixed:
Add a
defaultProps
getter on theUiProps
as an alternative way to retrieve a component's default props since instantiating the component class directly is an anti-pattern anyway.Testing suggestions:
@greglittlefield-wf @corwinsheahan-wf @evanweible-wf