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

Fix DatePickerAndroid error when passing "minDate: null" #21338

Conversation

MaxInMoon
Copy link

@MaxInMoon MaxInMoon commented Sep 26, 2018

It avoids an error when passing an options object with minDate: null. (see #21253)
Inspired by isObjectLike from Loadash.

Fixes #21253

@RSNara reviewer?

Test Plan:

This jsbin allow to test the old and the new _toMillis function.

You can observe that _toMillis doesn't throw error (fixed) and that if you decomment the _toMillis_before_pr call, it will generate an error as before.

Release Notes:

[ANDROID] [BUGFIX] [DatePickerAndroid] fix error when passing minDate: null in options

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

eslint found some issues. You may run yarn prettier or npm run prettier to fix these.

@@ -18,7 +18,7 @@ const DatePickerModule = require('NativeModules').DatePickerAndroid;
function _toMillis(options: Object, key: string) {
const dateVal = options[key];
// Is it a Date object?
if (typeof dateVal === 'object' && typeof dateVal.getMonth === 'function') {
if (dateVal != null && typeof dateVal == 'object' && typeof dateVal.getMonth === 'function') {

Choose a reason for hiding this comment

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

prettier/prettier: Replace dateVal·!=·null·&&·typeof·dateVal·==·'object'·&&·typeof·dateVal.getMonth·===·'function' with ⏎····dateVal·!=·null·&&⏎····typeof·dateVal·==·'object'·&&⏎····typeof·dateVal.getMonth·===·'function'⏎··

@@ -18,7 +18,7 @@ const DatePickerModule = require('NativeModules').DatePickerAndroid;
function _toMillis(options: Object, key: string) {
const dateVal = options[key];
// Is it a Date object?
if (typeof dateVal === 'object' && typeof dateVal.getMonth === 'function') {
if (dateVal != null && typeof dateVal == 'object' && typeof dateVal.getMonth === 'function') {

Choose a reason for hiding this comment

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

eqeqeq: Expected '===' and instead saw '=='.

@pull-bot
Copy link

Warnings
⚠️

📋 Release Notes - This PR may have incorrectly formatted Release Notes.

Generated by 🚫 dangerJS

@react-native-bot react-native-bot added Platform: Android Android applications. 🔶Components Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Sep 26, 2018
@elicwhite
Copy link
Member

Thanks for contributing! This file really looks like it needs to be flow typed. This change just feels like a small patch that might not completely handle all the cases of things not being typesafe.

Would you be willing to take a stab at flow typing this file to ensure we are properly handling all of the different edge cases of options?

@MaxInMoon
Copy link
Author

@TheSavior I'll do some flow-typing within the two next days as soon as I have a quiet moment.

Just two questions about optionsMs and options in the function below:

static async open(options: Object): Promise<Object> {
  const optionsMs = options;
  if (optionsMs) {
    _toMillis(options, 'date');
    _toMillis(options, 'minDate');
    _toMillis(options, 'maxDate');
  }
  return DatePickerModule.open(options);
}

1 - Why doing this: const optionsMs = options;?

2 - Why mutating the options object within _toMillis()? Would this not be a bad practice?

@elicwhite
Copy link
Member

The answer to both of your questions are "this is legacy code, I wouldn't want it written this way today". Good news! You can make it better! :D

Fortunately, if you flow type .open() then we shouldn't even need the _toMillis function.

@RSNara
Copy link
Contributor

RSNara commented Sep 27, 2018

Thanks for fixing this, @MaxInMoon! :)

Since this is a pretty simple change, I think we can merge this PR as is.

But as @TheSavior said, it'd be better if this file were flow-typed. The error you caught at run-time could have easily been caught at compile-time. Would you like to launch another PR? Feel free to refactor as you see fit.

Edit: Whoops. @MaxInMoon, it looks like I can't merge this because you haven't signed the CLA.

@hramos hramos added ✅Changelog and removed Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Jan 15, 2019
@cpojer
Copy link
Contributor

cpojer commented Jan 22, 2019

I agree with @TheSavior and @RSNara that this file should be flow typed instead of adding more runtime checks. Since there haven't been any updates in more than three months I'm gonna close this PR. I'm happy to reopen or land a new PR that adds Flow types to this module. Thank you for bringing this issue to our attention!

@cpojer cpojer closed this Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[0.57] [Android] DatePickerAndroid doesn't open because of null minDate
9 participants