fix(android): Resolve a crash under Android 7 #5206
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/invertase/react-native-firebase/Gfz89ZQ3GXwFkHPAnb4BHGybJpKa |
Codecov Report
@@ Coverage Diff @@
## master #5206 +/- ##
==========================================
+ Coverage 85.26% 88.87% +3.61%
==========================================
Files 109 109
Lines 3744 3744
Branches 360 360
==========================================
+ Hits 3192 3327 +135
+ Misses 481 370 -111
+ Partials 71 47 -24 |
|
Oh my! Thank you for letting us know, this is a new feature and is a great performance enhancement - when it's working - but we have no intention of requiring minSdkVersion 24 - I'll check into this and sorry for the bother. The releases prior to this (11.2.x) have no problems with them, it should be fine to revert until we've fixed |
|
Having tried this in another project I will likely go another direction, it has worked well for us: https://github.com/ankidroid/Anki-Android/pull/8361/files https://developer.android.com/studio/write/java8-support-table
It has not appreciably increased build times either A related failure (obviously) is that this was not caught and stopped in our CI, I'll alter our CI to use our minSdkVersion for the Android emulator (or the min possible - not sure 16 is still a reliable emulator, but something under 24 is, 21 at least I know works) vs the version it is on now |
|
@mikehardy I don't think it's the best way to support just 2 functions. That's because using If it is not necessary for the whole project and is not used frequently, you need to consider whether it is necessary to use it. It's just my opinion. |
|
@ifsnow that's a reasonable point, I understand the internal storage constraints of a lot of devices and am sensitive to that, plus we're a library not an app so we would be effectively making this decision (programming clarity vs apk size) for all consumers vs leaving it to them. Your code looks correct on first review and I'll look into the CI failure on the PR (likely unrelated), and merge it as is if it passes though. I want to alter our CI to reproduce the failure first though (API21 is available for testing in CI, as an emulator) so this can't happen again. Errors like this should never get past CI. |
|
Still working through CI flakiness so I can't merge this just yet, and I'll be traveling next 5hrs so this will likely be merged released overnight your time. Just giving you the update since it's nearly end of day there and this is a pretty serious issue. It'll be fixed by your next day though, thanks again for reporting this and posting the fix, I really appreciate it. |
Java lint was not being checked anywhere, and the NewApi check in particular would have stopped a crash. Add full lint configuration tuned for current status Cannot enable it on the modules or our modules may fail library-consumer's builds, but we may configure it on the sub-modules from our test app then check that Tested and working and would have correctly failed the NewApi violation shown in #5206
Java lint was not being checked anywhere, and the NewApi check in particular would have stopped a crash. Add full lint configuration tuned for current status Cannot enable it on the modules or our modules may fail library-consumer's builds, but we may configure it on the sub-modules from our test app then check that Tested and working and would have correctly failed the NewApi violation shown in #5206
|
v11.3.3 just went out with this fix, and I merged a CI change that will make sure it never happens again! |
Description
Related to : #4981
TaskExecutorService is using API level 24 Collection functions such as
removeIf,forEach.This can cause build failure or crash under Android 7.
I'm sure it needs to be modified to work with the lower minSdkVersion.
Checklist
AndroidiOSe2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
None