-
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-15063 Conditionally unconvert style and children props #698
Conversation
…t type For instance, when a JS component/library renders a Dart component and passes it a single value for children, or a JS object for style
/// The children that were passed in to this component when it was built. | ||
List children; | ||
List<dynamic> get children { |
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.
I inlined this implementation into the getter, as opposed to making it a utility function, so that I could do props.containsKey('children')
below
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
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 looks great! Awesome test coverage!
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
- w_table consumer test failures appear related to these changes
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
w_table consumer test failures were a red herring
+10
- Dart 2.7.2 / Stable CI builds and tests passed
- consumer tests with the exception of a few known flaky tests passed
@Workiva/release-management-pp
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
Sometimes, Dart code can read props that originate from JS code (e.g., a JS component rendering/cloning a Dart component), and in cases where the JS prop value doesn't match the typings on the OverReact prop getter, you get a runtime type error.
Two common cases of this are the
style
andchildren
props.style
props set by JS code are JS objects, which can't be casted to DartMap
s in theDomPropsMixin.style
andUbiquitousDomPropsMixin.style
getters.children
props are alwaysList
s for Dart components rendered by Dart code, but can be nullish or a singular value when rendered by JS components, and can cause cast errors in the typedchildren
prop getter.Changes
children
prop getter (ReactPropsMixin.children
, mixed intoUiProps
) to always be aList
style
prop getters (UbiquitousDomPropsMixin.style
, mixed intoUiProps
, andDomPropsMixin.style
) to convert JS object values to DartMap
sRelease Notes
Review
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: