Skip to content

Turn Flow strict mode on for DatePickerIOS #22105

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Turn Flow strict mode on for DatePickerIOS #22105

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a119642
add flow strict
nissy-dev Nov 4, 2018
debbcfd
fix annotation
nissy-dev Nov 4, 2018
1ce3ef8
fix error
nissy-dev Nov 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions Libraries/Components/DatePicker/DatePickerIOS.ios.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
* This is a controlled component version of RCTDatePickerIOS
*
* @format
* @flow
* @flow strict-local
*/

'use strict';
Expand All @@ -21,10 +21,15 @@ const View = require('View');
const requireNativeComponent = require('requireNativeComponent');

import type {ViewProps} from 'ViewPropTypes';
import type {SyntheticEvent} from 'CoreEventTypes';

const RCTDatePickerIOS = requireNativeComponent('RCTDatePicker');

type Event = Object;
type Event = SyntheticEvent<
$ReadOnly<{|
timestamp: string | number,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you link to the file(s) that shows that this is the type for timestamp from native?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like timestamp can only be a number:

react-native/React/Views/RCTDatePicker.m

Lines 32 to 37 in 1151c09

- (void)didChange
{
if (_onChange) {
_onChange(@{ @"timestamp": @(self.date.timeIntervalSince1970 * 1000.0) });
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK! I fixed.

|}>,
>;

type Props = $ReadOnly<{|
...ViewProps,
Expand Down Expand Up @@ -154,7 +159,7 @@ class DatePickerIOS extends React.Component<Props> {
? props.initialDate.getTime()
: undefined
}
locale={props.locale ? props.locale : undefined}
locale={props.locale}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the same behavior. React handles null (allowed by the flow type) differently than undefined. Was there a flow error that required this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes... I got this error...

Sketchy null check on string [1] which is potentially an empty string. Perhaps
you meant to check for null or undefined [2]? (sketchy-null-string)

I think this is a problem of flow. The situation is simlilar to this issue facebook/flow#4608

Copy link
Member

@elicwhite elicwhite Nov 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think to be semantically the same it needs to be changed to the following then.

locale={(props.locale != null && props.locale != "")  ? props.locale : undefined}

maximumDate={
props.maximumDate ? props.maximumDate.getTime() : undefined
}
Expand Down