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: Multiple FlutterError.onError calls in FlutterErrorIntegration #345

Merged
merged 9 commits into from
Mar 10, 2021

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Mar 9, 2021

📜 Description

Hold the a static reference to the default FlutterError.onError in FlutterErrorIntegration and restore it in integrations close() method.

💡 Motivation and Context

When doing multiple close/init on the SDK, multiple FlutterErrorIntegration instances are instantiated during application lifetime. Every one of them would replace FlutterError.onError function and reference the previous one. This resulted in a chain of calls, where errors were reported multiple times.

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

When doing multiple close/init on the SDK, multiple FlutterErrorIntegration instances were intantiated during application lifetime. Every one of them would replace FlutterError.onError function and reference the previouse one. This resulted in a chained call, where errors were reported multiple times.
@denrase denrase added the bug label Mar 9, 2021
@denrase denrase requested a review from marandaneto March 9, 2021 12:29
@denrase denrase self-assigned this Mar 9, 2021
@denrase denrase marked this pull request as ready for review March 9, 2021 12:30
@denrase denrase changed the title Fix: Multiple FlutterError.onError calls in FlutterErrorIntegration DRAFT: Fix: Multiple FlutterError.onError calls in FlutterErrorIntegration Mar 9, 2021
@denrase denrase changed the title DRAFT: Fix: Multiple FlutterError.onError calls in FlutterErrorIntegration Fix: Multiple FlutterError.onError calls in FlutterErrorIntegration Mar 9, 2021
@denrase
Copy link
Collaborator Author

denrase commented Mar 9, 2021

@denrase
Copy link
Collaborator Author

denrase commented Mar 9, 2021

@marandaneto @bruno-garcia Thanks for the detailed feedback, i incorporated the feedback and added one additional test. Should we also change something based on the noop suggestion from @bruno-garcia?

@denrase denrase requested a review from marandaneto March 9, 2021 16:44
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

LGTM

@marandaneto
Copy link
Contributor

@denrase looks like pod-lint is complaining out of the blue, but also on main, could you have a look please?

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

left a note about testing but other than that, LGTM.
thanks for fixing this.

@denrase
Copy link
Collaborator Author

denrase commented Mar 10, 2021

Ok, so apparently with Flutter >= 2.0.0, the Flutter/Flutter.xcframework contains slices for ios-x86_64-simulator and 'ios-armv7_arm64. So when setting x86_64 for the sim valid archs, it fails, as it is not part of the xcframework.

# Flutter.framework does not contain a i386 slice. Only x86_64 simulators are supported.
s.pod_target_xcconfig = { 'DEFINES_MODULE' => 'YES', 'VALID_ARCHS[sdk=iphonesimulator*]' => 'x86_64' }
  • When setting the flutter version to '1.22.4', it works as before.
  • When chaning to VALID_ARCHS[sdk=iphonesimulator*]' => 'ios-x86_64-simulator', the lint works, but we get the following warning:
- NOTE  | [iOS] xcodebuild:  warning: [CP] Unable to find matching .xcframework slice in 'Flutter/Flutter.xcframework Flutter framework ios-x86_64-simulator ios-armv7_arm64' for the current build architectures (arm64 x86_64 i386).

I'm not sure if we can ignore this warning, or if users will now run into issues when running on simulator. Do you have any suggestions @marandaneto? Think we should do this in a separate issue?

@denrase denrase merged commit 15e1f53 into main Mar 10, 2021
@denrase denrase deleted the fix-chained-flutter-onerror-methods branch March 10, 2021 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants