-
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
CPLAT-12196: @Component2 annotation parity for function components #655
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
44bd829
to
c0f7018
Compare
|
||
testComponentTypeChecking({ |
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 shared suite still exists, I just moved some tests out of it that didn't make sense to be shared
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
@@ -225,7 +225,7 @@ UiFactory<TProps> Function(UiFactory<TProps>) forwardRef<TProps extends UiProps> | |||
} | |||
|
|||
ReactComponentFactoryProxy hoc = react_interop.forwardRef(wrapProps, displayName: displayName); | |||
setComponentTypeMeta(hoc, isHoc: true, parentType: factory().componentFactory); | |||
setComponentTypeMeta(hoc.type, isHoc: true, parentType: factory().componentFactory.type); |
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.
Was this something you discovered was not working correctly - or is this necessary for these changes to work?
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.
It was working fine before; this was necessary since I changed setComponentTypeMeta
, a private API, to expect the raw type instead of the proxy.
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! And I can QA in the morning!
It looks like some related tests are failing. |
Co-authored-by: Aaron Lademann <aaron.lademann@workiva.com> Co-authored-by: joebingham-wk <46691367+joebingham-wk@users.noreply.github.com>
e9c0d6b
28c5c3c
to
2429cdc
Compare
2429cdc
to
bf6ffef
Compare
All issues have been addressed! |
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.
+10
- code looks good
- QA'd by:
- Ensuring wsd test branch passes with latest changes
- Mimicked a WSD subtype example to verify it worked as expected
@Workiva/release-management-p |
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 from RM
Motivation
The
@Component2
annotation can't be used with function components, preventing consumers from specifying arguments likeisWrapper
andsubtypeOf
.Changes
setTypeMeta
extension method toUiFactory
, providing parity with the@Component2
annotation for function componentssetComponentTypeMeta
andComponentTypeMeta
to use raw component types in case theReactComponentFactoryProxy
isn't availableExamples of equivalent configuration for class-based vs function components:
Release Notes
UiFactory.setComponentMeta
, which brings@Component2
annotation configuration to function componentsReview
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: