Rerender of android picker no longer clears users selection

[aleksanb]

Summary

React may rerender components whenever it feels like, even when props haven't changed.
Calling update on the native android picker will always change its currently selected value.
This happens even if the user has changed the selection in the picker.
By only updating the selected value of the picker if it actually differs from last render,
we avoid the user suddenly having their selection change seemingly randomly.

iOS already has a debounce equivalent to this one implemented, where
only actually changed datetime values are forwarded to native
components, see RNDateTimePicker.m#setDate.

Closes #182
Closes #235

Test Plan

The bug is demonstrated by rendering a date or timepicker on android, and having the component rerender. rerendering can be triggered by calling a settimeout in render calling setstate , or calling a setinterval in the constructor setting state.

One can probably modify the example code from the README, adding the settimeout i added in render here.
Open the datepicker, change your value, and observe the value will be reset without you clicking anything. With this patch however, it will stop changing unless date is actually changed to a different value.

import React, {useState} from 'react';
import {View, Button, Platform} from 'react-native';
import DateTimePicker from '@react-native-community/datetimepicker';

export const App = () => {
  const [date, setDate] = useState(new Date(1598051730000));
  const [mode, setMode] = useState('date');
  const [show, setShow] = useState(false);

  const onChange = (event, selectedDate) => {
    const currentDate = selectedDate || date;
    setShow(Platform.OS === 'ios');
    setDate(currentDate);
  };

  const showMode = (currentMode) => {
    setShow(true);
    setMode(currentMode);
  };

  const showDatepicker = () => {
    showMode('date');
  };

  const showTimepicker = () => {
    showMode('time');
  };

  setTimeout(() => setDate(old => old), 1000);

  return (
    <View>
      <View>
        <Button onPress={showDatepicker} title="Show date picker!" />
      </View>
      <View>
        <Button onPress={showTimepicker} title="Show time picker!" />
      </View>
      {show && (
        <DateTimePicker
          testID="dateTimePicker"
          value={date}
          mode={mode}
          is24Hour={true}
          display="default"
          onChange={onChange}
        />
      )}
    </View>
  );
};

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Compatibility

OS	Implemented
iOS	✅ ❌
Android	✅ ❌

Checklist

• I have tested this on a device and a simulator
• I added the documentation in README.md
• I updated the typed files (TS and Flow)
• I added a sample use of the API in the example project (example/App.js)
• I have added automated tests, either in JS or e2e tests, as applicable

[aleksanb]

whops i made these too early, going to fix some fmt, doc etc first before marking as not draft.

[aleksanb]

ready for review now :)

[melvynhills]

Just installed the lib and literally directly stumbled upon this very problem. Quite crazy to me this hasn't been discovered/fixed before 😳 But anyway thanks for this fix, and of course to all maintainers as well.

[luancurti]

@aleksanb Can you add the Test Plan with steps to test this changes?

[aleksanb]

@aleksanb Can you add the Test Plan with steps to test this changes?

Yes. Updated.

[vonovak]

Thank you for the PR.

@melvynhills this is one of the issues I'm well aware of but haven't been motivated to really fix it 🤷 . That is why we're looking for help in #313.

I think the underlying problem is that we're exposing the date picking dialog as if it were a component, hiding it under what looks like a declarative api, but in fact is an imperative api, and I think it should be an imperative api, just like it was in the react native core (https://reactnative.dev/docs/datepickerandroid).

I mean, look at this:

datetimepicker/src/datetimepicker.android.js

Line 69 in 7d1d70f

picker.then(

we're opening the picker directly in the render method. It's gross. If anything, it should be in an effect. But really, component is a wrong abstraction here imho.

That is, imho, the api should be

try {
  const {
    action,
    year,
    month,
    day
  } = await DatePickerAndroid.open({
    date: new Date(2020, 4, 25)
  });
  if (action !== DatePickerAndroid.dismissedAction) {
    // Selected year, month (0-11), day
  }
} catch ({ code, message }) {
  console.warn('Cannot open date picker', message);
}

and not

<DateTimePicker
  value={date}
  display="default"
  onChange={onChange}
/>

The dialog is much more like an Alert, and not like eg. an ActivityIndicator. If you look at the docs, the date picker is in the "api" section, and ActivityIndicator is in "components" section. We should imo keep it that way.

so the solution I have envisioned would be to implement #14

and then the dialog would be opened imperatively, as I have shown above. This would also fix #326

What do you folks think about this?

[melvynhills]

@vonovak Thanks for the clarification.

100% agree with you, the declarative API seems very wrong to handle such an imperative API.

[aleksanb]

The question becomes what to do in the meantime.
Rewriting the lib to again expose an imperative api will take time, and effort, and someone to rewrite the library. It will break downstream consumers, even though it might reflect the underlying apis better.

This patch fixes bugs in the current declarative api, and will bring improvements to all current users.

[vonovak]

The question becomes what to do in the meantime.

@aleksanb does that mean you also agree with what I outlined in my previous comment?

Rewriting the lib to again expose an imperative api will take time

it's already here: https://github.com/react-native-datetimepicker/datetimepicker/blob/master/src/timepicker.android.js and https://github.com/react-native-datetimepicker/datetimepicker/blob/master/src/datepicker.android.js so it's just the matter of exposing it and documenting it. It's a job for 1.5 hours at most I think (documentation will be the harder part 😂 ).

This patch fixes bugs in the current declarative api, and will bring improvements to all current users.

I agree that would be nice.

What do you think about moving the open call

datetimepicker/src/datetimepicker.android.js

Line 69 in 7d1d70f

picker.then(

into an effect so that it only happens upon mount? Wouldn't that solve the issue as well with minimum changes?

Thanks!

[aleksanb]

The question becomes what to do in the meantime.

@aleksanb does that mean you also agree with what I outlined in my previous comment?

I think that having it as an imperative api is an okay choice. For my current codebase, we've already integrated the current declarative component-based api. One thing to consider then, is what to do about iOS. iOS would still have components you integrate yourself. This is ok, as there's no requirement to expose a unified cross-platform api, as platforms are different, but something to think about.

What do you think about moving the open call

datetimepicker/src/datetimepicker.android.js

Line 69 in 7d1d70f

picker.then(

Moving this code to a useefect that only runs on mount would stop this bug, but prohibit the current feature that you can update the selection of the picker from javascript while it is open by changing the value prop to a different date. The code today supports this use case. Whether it is one that is useful in practice is a different question, but only running on mount would break that.

[vonovak]

Moving this code to a useefect that only runs on mount would stop this bug, but prohibit the current feature that you can update the selection of the picker from javascript while it is open by changing the value prop to a different date

well, how about making the effect depend on the value prop, then? I think it'd communicate the intent clearly

[stianjensen]

Moving this code to a useefect that only runs on mount would stop this bug, but prohibit the current feature that you can update the selection of the picker from javascript while it is open by changing the value prop to a different date

well, how about making the effect depend on the value prop, then? I think it'd communicate the intent clearly

Yes, that will work. Just make sure that if you call dismiss/close automatically in the cleanup of the useEffect, to only do that in the final useEffect when the component unmounts, so you will need two useEffects for this. One that runs open on every change to value, and one that only ensures to run dismiss in the unmount-case.

[vonovak]

@stianjensen

Yes, that will work. Just make sure that if you call dismiss/close automatically in the cleanup of the useEffect, to only do that in the final useEffect when the component unmounts

that is a good point but we currently don't have a dismiss method, so we don't need to worry about it at this point 🙂

@aleksanb what do you think about the proposed solution (useffect that depends on value prop). Would you update the PR to be done that way? Thanks!

[vonovak]

🎉 This issue has been resolved in version 3.0.9 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀