Skip to content
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

Merged
merged 6 commits into from
Oct 21, 2022
Merged

Fix crash by disabling Flipper on API 22 and below #7428

merged 6 commits into from
Oct 21, 2022

Conversation

jonnyandrew
Copy link
Contributor

@jonnyandrew jonnyandrew commented Oct 21, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

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

  • Launch the app on an emulator running Android 5
  • Observe no crash

Tested devices

  • Physical
  • Emulator
  • OS version(s): 21

Checklist

@jonnyandrew jonnyandrew changed the title Disable Flipper on API 21 and below Fix crash by disabling Flipper on API 21 and below Oct 21, 2022
@jonnyandrew jonnyandrew changed the title Fix crash by disabling Flipper on API 21 and below Fix crash by disabling Flipper on API 22 and below Oct 21, 2022
@jonnyandrew jonnyandrew marked this pull request as ready for review October 21, 2022 11:08
Copy link
Member

@jmartinesp jmartinesp left a 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.

@jmartinesp
Copy link
Member

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?

Copy link
Member

@bmarty bmarty left a 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
}
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changelog.d/7428.bugfix Outdated Show resolved Hide resolved
@jonnyandrew jonnyandrew requested a review from a team October 21, 2022 13:00
jonnyandrew and others added 2 commits October 21, 2022 14:01
Co-authored-by: Benoit Marty <benoit.marty@gmail.com>
override fun networkInterceptor() = FlipperOkhttpInterceptor(networkFlipperPlugin)
override fun networkInterceptor() =
FlipperOkhttpInterceptor(networkFlipperPlugin)
.takeIf { isEnabled }
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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 👍

Copy link
Member

@bmarty bmarty left a 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.

@bmarty
Copy link
Member

bmarty commented Oct 21, 2022

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?

I will check the repo settings.

@jonnyandrew can you add yourself in this list? You can do it in this PR, this is fine.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jonnyandrew jonnyandrew merged commit 31811bb into element-hq:develop Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants