-
Notifications
You must be signed in to change notification settings - Fork 782
Fix crash by disabling Flipper on API 22 and below #7428
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 crash by disabling Flipper on API 22 and below #7428
Conversation
Due to a bug: facebook/flipper#3572
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.
The code LGTM, let's see if we can make the checks pass.
Danger seems to be failing consistently. Maybe it's because @jonnyandrew is in the orgs but still doesn't have write access to this repo for some reason? |
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.
LGTM, some remarks though.
// https://github.com./facebook/flipper/issues/3572 | ||
if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.LOLLIPOP_MR1) { | ||
return | ||
} |
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.
Why this is not done above the line SoLoader.init(context, false)
?
Also I would also update the fun networkInterceptor()
to return null if Build.VERSION.SDK_INT <= Build.VERSION_CODES.LOLLIPOP_MR1
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.
I put it here to keep this logic at the same point as the existing check (on the line immediately after this one).
In the docs SoLoader.init()
is always called regardless of whether Flipper is enabled but I'm not sure if there's any reason for that.
The same is true of the networkInterceptor()
but I'd agree, it should return null if Flipper is disabled.
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.
Co-authored-by: Benoit Marty <[email protected]>
override fun networkInterceptor() = FlipperOkhttpInterceptor(networkFlipperPlugin) | ||
override fun networkInterceptor() = | ||
FlipperOkhttpInterceptor(networkFlipperPlugin) | ||
.takeIf { isEnabled } |
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.
Not a big deal, but creating on object for nothing is a bit weird. I would change to
override fun networkInterceptor() = if (isEnabled) FlipperOkhttpInterceptor(networkFlipperPlugin) else null
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.
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.
Agreed - and takeIf
is not very readable either 👍
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 update.
I will check the repo settings. @jonnyandrew can you add yourself in this list? You can do it in this PR, this is fine. |
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!
Type of change
Content
Disable Flipper on API 22 and below as it causes a crash immediately on app startup.
Motivation and context
Screenshots / GIFs
N/A
Tests
Tested devices
Checklist