search

wmcmahan / react-native-calendar-events

📆 React Native Module for iOS and Android Calendar Events
MIT License
906 stars 292 forks source link

Generic anti-crash approach for android #352

Closed MoOx closed 3 years ago

MoOx commented 3 years ago

We have a lot of issues for crash on android (too many for me to list them all).

Like we did on iOS, we should avoid crash at all cost and prefer promise failure. I am not familiar with java rn module (but I wasn't for iOS and handled this in #314) and think we should do something similar: skip things for minor failure (eg: event serialisation details) or promise rejections for method call level failure.

Any help to discuss/start this is appreciated!

steven-sh commented 3 years ago

@MoOx @wmcmahan I'd be happy to look into this (guess what 4 of my top 5 crashes on Android are!).

For the approach, in general I prefer to fail rather than skip even for minor things. I find that not doing this can often lead to crashes later on that are then hard to trace to the source (e.g. if an event ends up without an expected field and we then get a JS crash when trying to read the value). Did you have anywhere you were specifically looking to skip?

MoOx commented 3 years ago

What I have in mind for the long run is to add an errors field in serialised event, to pop errors for each event if any. Because when processing tons of events (like I do) it's really annoying if one event fails on thousands to have to say "sorry" to the end user... Anyway, that can be done in parallel to a global approach. Really similar to what I did for iOS in #314: just wrap all call with a try/catch (of whatever is relevant for java/android) to never crash, but send a promise rejection instead.

steven-sh commented 3 years ago

Ah that makes sense. For now it sounds like a blanket try/catch on all the react methods would be useful so I'll give that a go and make a PR.