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

Improve FutureOr usage only when necessary #1385

Merged
merged 8 commits into from Apr 20, 2023

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Apr 7, 2023

📜 Description

We should only use FutureOr<T> when we don't know its type.
We should always use Future<T> or T when we know the return type.
We should only await if its a Future, we can check the return type of a FutureOr<T> method.
We always implement an interface and only extend when we need to make use of its default methods.
Tests should only be async if its awaiting for a Future.
We should always honor the return type of an interface, for example, SentryEvent? instead of SentryEvent.

💡 Motivation and Context

Also fixes #1336
Screenshots and View hierarchy should only be added to errors and not transactions.

💚 How did you test it?

Tests already exist.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@marandaneto marandaneto changed the title Improve FutureOr usage usage only when necessary Improve FutureOr usage only when necessary Apr 7, 2023
@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (99864ad) 89.91% compared to head (50c9cb5) 89.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1385      +/-   ##
==========================================
+ Coverage   89.91%   89.92%   +0.01%     
==========================================
  Files         179      179              
  Lines        5759     5765       +6     
==========================================
+ Hits         5178     5184       +6     
  Misses        581      581              
Impacted Files Coverage Δ
...rocessor/enricher/io_enricher_event_processor.dart 93.61% <ø> (ø)
...cessor/exception/io_exception_event_processor.dart 86.36% <ø> (ø)
dart/lib/src/isolate_error_integration.dart 100.00% <ø> (ø)
dart/lib/src/run_zoned_guarded_integration.dart 69.69% <ø> (ø)
...nt_processor/native_app_start_event_processor.dart 93.33% <ø> (ø)
.../lib/src/integrations/debug_print_integration.dart 92.85% <ø> (-0.48%) ⬇️
...ib/src/integrations/flutter_error_integration.dart 100.00% <ø> (ø)
...ib/src/integrations/load_contexts_integration.dart 99.02% <ø> (ø)
.../src/integrations/load_image_list_integration.dart 96.15% <ø> (ø)
...lib/src/integrations/load_release_integration.dart 100.00% <ø> (ø)
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 379.82 ms 442.63 ms 62.82 ms
Size 6.06 MiB 7.03 MiB 993.46 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
11fb408 320.10 ms 380.24 ms 60.14 ms
fdac48a 329.50 ms 396.46 ms 66.96 ms
a7acb24 301.00 ms 357.38 ms 56.38 ms
eecbbca 324.37 ms 352.49 ms 28.12 ms
24f71aa 358.49 ms 455.90 ms 97.41 ms
2f8f173 323.31 ms 373.29 ms 49.97 ms
6957bfd 325.88 ms 380.30 ms 54.43 ms
6325c3b 339.33 ms 409.86 ms 70.53 ms
d7758e8 300.12 ms 349.88 ms 49.76 ms
40680d3 323.55 ms 390.29 ms 66.73 ms

App size

Revision Plain With Sentry Diff
11fb408 6.06 MiB 7.10 MiB 1.04 MiB
fdac48a 6.06 MiB 7.09 MiB 1.03 MiB
a7acb24 5.94 MiB 6.95 MiB 1.01 MiB
eecbbca 5.94 MiB 6.89 MiB 975.78 KiB
24f71aa 6.06 MiB 7.03 MiB 990.30 KiB
2f8f173 5.94 MiB 6.95 MiB 1.01 MiB
6957bfd 5.94 MiB 6.95 MiB 1.01 MiB
6325c3b 5.94 MiB 6.96 MiB 1.02 MiB
d7758e8 5.94 MiB 6.95 MiB 1.01 MiB
40680d3 6.06 MiB 7.03 MiB 989.25 KiB

Previous results on branch: chore/improve-futureor-usage

Startup times

Revision Plain With Sentry Diff
1d53081 304.76 ms 370.33 ms 65.58 ms
ade80eb 296.06 ms 339.60 ms 43.53 ms
c4eec5c 301.63 ms 366.76 ms 65.13 ms

App size

Revision Plain With Sentry Diff
1d53081 6.06 MiB 7.03 MiB 993.29 KiB
ade80eb 6.06 MiB 7.03 MiB 993.42 KiB
c4eec5c 6.06 MiB 7.03 MiB 993.30 KiB

@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1266.35 ms 1277.18 ms 10.84 ms
Size 8.10 MiB 9.17 MiB 1.08 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
453e1bc 1294.78 ms 1308.10 ms 13.33 ms
633cf2e 1257.96 ms 1275.73 ms 17.77 ms
322aa66 1251.68 ms 1275.52 ms 23.84 ms
aa950e9 1275.17 ms 1295.33 ms 20.16 ms
a49594a 1284.83 ms 1313.29 ms 28.45 ms
3e5ee37 1248.25 ms 1265.38 ms 17.13 ms
0be962b 1264.10 ms 1281.16 ms 17.06 ms
613760b 1263.10 ms 1277.27 ms 14.16 ms
134c9f8 1284.48 ms 1306.18 ms 21.70 ms
9928a74 1249.98 ms 1269.08 ms 19.10 ms

App size

Revision Plain With Sentry Diff
453e1bc 8.16 MiB 9.16 MiB 1.00 MiB
633cf2e 8.15 MiB 9.12 MiB 986.26 KiB
322aa66 8.15 MiB 9.12 MiB 992.53 KiB
aa950e9 8.16 MiB 9.17 MiB 1.01 MiB
a49594a 8.16 MiB 9.16 MiB 1.00 MiB
3e5ee37 8.15 MiB 9.12 MiB 986.23 KiB
0be962b 8.10 MiB 9.16 MiB 1.07 MiB
613760b 8.15 MiB 9.13 MiB 1000.46 KiB
134c9f8 8.16 MiB 9.16 MiB 1.01 MiB
9928a74 8.16 MiB 9.17 MiB 1.01 MiB

Previous results on branch: chore/improve-futureor-usage

Startup times

Revision Plain With Sentry Diff
ade80eb 1257.78 ms 1285.20 ms 27.43 ms
c4eec5c 1275.67 ms 1285.82 ms 10.14 ms
1d53081 1235.35 ms 1252.76 ms 17.41 ms

App size

Revision Plain With Sentry Diff
ade80eb 8.10 MiB 9.17 MiB 1.08 MiB
c4eec5c 8.10 MiB 9.17 MiB 1.08 MiB
1d53081 8.10 MiB 9.17 MiB 1.08 MiB

@krystofwoldrich
Copy link
Member

@marandaneto Would it be possible to check this by a lint rule?

@marandaneto
Copy link
Contributor Author

@marandaneto Would it be possible to check this by a lint rule?

only if we make the linting rules ourselves, they don't exist just yet afaik.

@marandaneto marandaneto marked this pull request as ready for review April 18, 2023 07:55
Copy link
Member

@krystofwoldrich krystofwoldrich 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 marandaneto merged commit 5f30925 into main Apr 20, 2023
93 checks passed
@marandaneto marandaneto deleted the chore/improve-futureor-usage branch April 20, 2023 14:18
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.

Remove View Hierarchy from Flutter Web
2 participants