Skip to content

Conversation

@denrase
Copy link
Collaborator

@denrase denrase commented May 10, 2022

📜 Description

Sync scope to native. This is a breaking change, as some methods of scope.dart now return a future which can be awaited on.

💡 Motivation and Context

Closes #194

💚 How did you test it?

Unit tests.

📝 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

Check on web wether data is synced correctly to the native layer

@codecov-commenter
Copy link

codecov-commenter commented May 10, 2022

Codecov Report

Merging #858 (de28717) into main (1a418c5) will increase coverage by 0.25%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #858      +/-   ##
==========================================
+ Coverage   89.68%   89.94%   +0.25%     
==========================================
  Files         104        9      -95     
  Lines        3160      169    -2991     
==========================================
- Hits         2834      152    -2682     
+ Misses        326       17     -309     
Impacted Files Coverage Δ
dart/lib/src/scope.dart
dart/lib/src/sentry_client.dart
dart/lib/src/sentry_options.dart
dart/lib/src/protocol/sentry_browser.dart
dart/lib/src/protocol/sentry_operating_system.dart
dart/lib/src/transport/rate_limiter.dart
...event_processor/deduplication_event_processor.dart
dart/lib/src/protocol/sdk_info.dart
dart/lib/src/protocol/sentry_request.dart
dart/lib/src/protocol/span_status.dart
... and 85 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a418c5...de28717. Read the comment docs.

@marandaneto
Copy link
Contributor

We need to avoid crumbs duplication for iOS, on React Native we have a workaround for Android, See https://github.com/getsentry/sentry-react-native/blob/main/src/js/wrapper.ts#L100-L110

@marandaneto
Copy link
Contributor

marandaneto commented May 10, 2022

@bruno-garcia @denrase So the main question here is: FutureOr<type> works well, it's up to the user either to await or not, but it's a breaking change of the API in 2 ways, the behavior, since by default is a sync call, and after this change is fire and forget.
The other breaking change is: for making it a FutureOr<type> we cannot assign values to the reference directly such as scope.user = User but we rather need a new method such as FutureOr<User> setUser(User user), this one is a compile error.
I believe if we go with this one, we'd need to bump a major version, thoughts?

The other option is to make it fire and forget by default and document as best effort.

@denrase denrase requested a review from marandaneto May 24, 2022 08:38
@marandaneto
Copy link
Contributor

@denrase

/github/workspace/flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt:47:16: The function onMethodCall appears to be too complex (18). Defined complexity threshold for methods is set to '15' [ComplexMethod]
/github/workspace/flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt:2[6](https://github.com/getsentry/sentry-dart/runs/6652160882?check_suite_focus=true#step:4:7)5:15: Function setUser is nested too deeply. [NestedBlockDepth]

We can either fix it if possible or regenerate the baseline file flutter/config/detekt-bl.xml

@marandaneto
Copy link
Contributor

@denrase left a few comments but other than that, all good, thanks for doing this.

@denrase
Copy link
Collaborator Author

denrase commented Jun 7, 2022

In https://detekt.dev/docs/rules/complexity/#complexmethod we could setup ignoreSingleWhenExpression to true, but i don't see a yaml file for setup.

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.

Thanks @denrase

@marandaneto marandaneto merged commit fe6fcc8 into main Jun 8, 2022
@marandaneto marandaneto deleted the feat/sync-scope-to-native branch June 8, 2022 08:10
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.

Sentry.configureScope not synchronize in native

6 participants