-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
[flow-strict] Flow strict-local in TimePickerAndroid.android.js #22154
[flow-strict] Flow strict-local in TimePickerAndroid.android.js #22154
Conversation
Generated by 🚫 dangerJS |
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.
Thanks for this PR! :)
But I think we should make a few changes before we land it.
Also, I see that you launched another PR for the iOS version of this component. We should probably pull those changes into this PR and export the shared types into a separate file to avoid duplicating types.
return TimePickerModule.open(options); | ||
} | ||
|
||
/** | ||
* A time has been selected. | ||
*/ | ||
static get timeSetAction() { | ||
static getTimeSetAction(): string { |
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.
Instead of changing this getter into a function, which breaks the public API of this component, let's instead turn it into a covariant static property, which makes it read-only:
static +timeSetAction = 'timeSetAction'
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.
@RSNara Thank you for your advice. I've modified this.
return 'timeSetAction'; | ||
} | ||
/** | ||
* The dialog has been dismissed. | ||
*/ | ||
static get dismissedAction() { | ||
static getDismissedAction(): string { |
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.
Let's use a covariant static property here as well:
static +dismissedAction = 'dismissedAction';
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 think you forgot to make the static properties covariant.
Also, as I suggested before, I think you should merge #22155 into this PR.
static getDismissedAction(): string { | ||
return 'dismissedAction'; | ||
} | ||
static dismissedAction = 'dismissedAction'; |
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 think you forgot to make this covariant:
static +dismissedAction: string = 'dismissedAction';
You need the +
after the static
and before dismissedAction
.
static getTimeSetAction(): string { | ||
return 'timeSetAction'; | ||
} | ||
static timeSetAction = 'timeSetAction'; |
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.
Same here. You forgot to make this covariant:
static +timeSetAction: string = 'timeSetAction';
Summary: Related to #22100 . I had this issue before(#22154 & #22172). Turn Flow strict mode on for Libraries/Components/TimePickerAndroid/TimePickerAndroid.android.js. - [x] npm run prettier - [x] npm run flow - [x] npm run flow-check-ios - [x] npm run flow-check-android - [x] npm run lint - [x] npm run test - [x] ./scripts/run-android-local-unit-tests.sh [GENERAL][ENHANCEMENT][TimePickerAndroid.android.js] - apply flow strict-local Pull Request resolved: #22188 Reviewed By: TheSavior Differential Revision: D12972356 Pulled By: RSNara fbshipit-source-id: 838604a791dfdc86bacf8b49f6c399920a3f57bc
Summary: Related to facebook#22100 . I had this issue before(facebook#22154 & facebook#22172). Turn Flow strict mode on for Libraries/Components/TimePickerAndroid/TimePickerAndroid.android.js. - [x] npm run prettier - [x] npm run flow - [x] npm run flow-check-ios - [x] npm run flow-check-android - [x] npm run lint - [x] npm run test - [x] ./scripts/run-android-local-unit-tests.sh [GENERAL][ENHANCEMENT][TimePickerAndroid.android.js] - apply flow strict-local Pull Request resolved: facebook#22188 Reviewed By: TheSavior Differential Revision: D12972356 Pulled By: RSNara fbshipit-source-id: 838604a791dfdc86bacf8b49f6c399920a3f57bc
Summary: Post: https://fb.workplace.com/groups/rnsyncsquad/permalink/879923262900946/ This sync includes the following changes: - **[fc3b6a411](facebook/react@fc3b6a411 )**: Fix a few typos ([#22154](facebook/react#22154)) //<Bowen>// - **[986d0e61d](facebook/react@986d0e61d )**: [Scheduler] Add tests for isInputPending ([#22140](facebook/react#22140)) //<Andrew Clark>// - **[d54be90be](facebook/react@d54be90be )**: Set up test infra for dynamic Scheduler flags ([#22139](facebook/react#22139)) //<Andrew Clark>// - **[7ed0706d7](facebook/react@7ed0706d7 )**: Remove the warning for setState on unmounted components ([#22114](facebook/react#22114)) //<Dan Abramov>// - **[9eb2aaaf8](facebook/react@9eb2aaaf8 )**: Fixed ReactSharedInternals export in UMD bundle ([#22117](facebook/react#22117)) //<Brian Vaughn>// - **[bd255700d](facebook/react@bd255700d )**: Show a soft error when a text string or number is supplied as a child to non text wrappers ([#22109](facebook/react#22109)) //<Sota>// Changelog: [General][Changed] - React Native sync for revisions 424fe58...bd5bf55 jest_e2e[run_all_tests] Reviewed By: yungsters Differential Revision: D30485521 fbshipit-source-id: c5b92356e9e666eae94536ed31b8de43536419f8
Related to #22100
Turn Flow strict mode on for Libraries/Components/TimePickerAndroid/TimePickerAndroid.android.js.
Test Plan:
Changelog:
[GENERAL][ENHANCEMENT][TimePickerAndroid.android.js] - apply flow strict-local