-
-
Notifications
You must be signed in to change notification settings - Fork 449
fix(ios): avoid min>max date picker crash #1009
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(ios): avoid min>max date picker crash #1009
Conversation
| } | ||
|
|
||
| if (oldPickerProps.maximumDate != newPickerProps.maximumDate) { | ||
| picker.maximumDate = convertJSTimeToDate(newPickerProps.maximumDate); |
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.
while my reproducer is a naive example, i believe the behaviour leading to the crash was less obvious b/c of this UIDatePicker instance re-update logic:
if a datepicker is rendered (inline on a screen or sheet for example, as is the case in the reproducer), and the component is re-rendered, the crash would occur when either minimumDate or maximumDate is updated in a way that breaks the expected constraint order (min < max) but the prior picker value for one of the props is not unset first
since these conditionals were evaluated independently this seems like the culprit, hence why I've grouped them together to validate them together
|
Looks like this also might fix #1008 |
|
Thanks for this PR. This bug is really problematic for us since our production users are experiencing a lot of crashes in our datepickers modals/screens. Looking forward to see it merged ! |
|
@yonitou i don't know enough about your setup or specific issue but i don't see a problem with referencing the PR. we've applied the patch but are still waiting on rolling it out to prod. personally i'd apply it as a patch to whatever version you're on since the NPM package content won't be the same as whatever you pull in from a github reference |
vonovak
left a comment
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 the PR and the context, really helps with evaluating the code 🙏
08db857
into
react-native-datetimepicker:master
|
tested it, but it still doesn't work correctly when the I guess the logic should be more like that: |
|
@dzbrozek thanks for the comment, please feel free to open a PR for a fix, explaining how to get the issue you're seeing. Thank you! 🙏 |
Summary
closes #1006
closes #1008
This change fixes a crash on iOS when the underlying UIDatePicker receives minimumDate > maximumDate by:
This is the error that shows after this change in dev mode when this happens (before it just crashes):
Original Crash
Test Plan
Reproducer used in screenshots below: https://github.com/sterlingwes/date-picker-min-max-repro
The reproducer tests the following scenarios:
Behaviour before fixes
Behaviour after fixes
What's required for testing (prerequisites)?
You can either:
What are the steps to reproduce (after prerequisites)?
To reproduce the crash in the example app, tap the new button "set min > max (error)" then tap the picker date and it should crash. If you test on this branch that includes the changes in
ios/fabric/RNDateTimePickerComponentView.mmand insrc/utils.js, you should instead see the logbox error shown first above.Compatibility
The native crash was only evident on iOS, but the JS validation approach taken here ensures the expectation is aligned across both.
Checklist
README.mdexample/App.js)