-
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-9715 Fix accidental jsification of Map/Function Dart props when using connect
#461
Conversation
/// without any conversion needed by JS Components, and props are are fed directly | ||
/// into Dart code (e.g., those passed into mapStateToPropsWithOwnProps/areOwnPropsEqual) | ||
/// without needing unwrapping/conversion. | ||
final hocFactoryProxy = ReactDartComponentFactoryProxy2(hoc); |
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.
@aaronlademann-wf We might want to consider just doing this for forwardRef
as well (conditionally if the provided factory is a Dart component), since I think this jsification issue might still exist in react-dart 5.4.0-wip.
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
connect
connect
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
|
||
test('without converting the props whatsoever', () { | ||
// Test functions/Maps to ensure they're not allowInterop'd, | ||
// test event handlers to ensure they're not oterwise converted |
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.
#nit typo
// test event handlers to ensure they're not oterwise converted | |
// test event handlers to ensure they're not otherwise converted |
QA +1, Verified through https://github.com/Workiva/graph_ui/pull/1058 that the properties correctly get passed through connect as Dart props and not the previous js props |
@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
Wrapping Dart components in
connect
unexpectedly jsified props, causingMap
props to show up as JS Objects when read by the component.Changes
Release Notes
connect
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review: @aaronlademann-wf @joebingham-wk @sydneyjodon-wk
FYI: @matthewnitschke-wk @kealjones-wk
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: