Skip to content
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

Closed

Conversation

binaryta
Copy link
Contributor

@binaryta binaryta commented Nov 5, 2018

Related to #22100

Turn Flow strict mode on for Libraries/Components/TimePickerAndroid/TimePickerAndroid.android.js.

Test Plan:

  • npm run prettier
  • npm run flow
  • npm run flow-check-ios
  • npm run flow-check-android
  • npm run lint
  • npm run test
  • ./scripts/run-android-local-unit-tests.sh

Changelog:

[GENERAL][ENHANCEMENT][TimePickerAndroid.android.js] - apply flow strict-local

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. ✅pr adds tests labels Nov 5, 2018
@pull-bot
Copy link

pull-bot commented Nov 5, 2018

Warnings
⚠️

📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

@RSNara RSNara changed the title Flow strict-local in TimePickerAndroid.android.js [flow-strict] Flow strict-local in TimePickerAndroid.android.js Nov 5, 2018
Copy link
Contributor

@RSNara RSNara left a 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 {
Copy link
Contributor

@RSNara RSNara Nov 5, 2018

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'

Copy link
Contributor Author

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 {
Copy link
Contributor

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';

Copy link
Contributor

@RSNara RSNara left a 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';
Copy link
Contributor

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';
Copy link
Contributor

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';

@binaryta binaryta closed this Nov 6, 2018
facebook-github-bot pushed a commit that referenced this pull request Nov 21, 2018
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
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
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
facebook-github-bot pushed a commit that referenced this pull request Aug 24, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: TimePickerAndroid CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Flow Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants