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.

Sign up for GitHub

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

Jump to bottom

fix: setContext correctly processes non-plain JS object #4168

Merged
merged 5 commits into from
Oct 15, 2024
Merged

fix: setContext correctly processes non-plain JS object #4168

merged 5 commits into from
Oct 15, 2024

Conversation

krystofwoldrich
Copy link
Member

📢 Type of change

  • Bugfix
  • Enhancement
  • Refactoring

📜 Description

This PR adds more check and conversions for setExtra and setContext to allow sync from JS to Platform runtime without crashes.

💡 Motivation and Context

fixes: #4165

💚 How did you test it?

unit tests, sample app

📝 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
  • All tests passing
  • No breaking changes

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 14, 2024 •
edited
Loading

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1220.42 ms 1221.67 ms 1.25 ms
Size 2.36 MiB 3.14 MiB 793.46 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
ad6c299+dirty 1244.76 ms 1260.10 ms 15.34 ms
5a22220+dirty 1209.49 ms 1220.94 ms 11.45 ms
0db0c72+dirty 1275.02 ms 1285.84 ms 10.82 ms
d0bf494+dirty 1289.40 ms 1298.40 ms 9.00 ms
76d1baf+dirty 1244.10 ms 1268.52 ms 24.42 ms
2534337+dirty 1225.08 ms 1230.26 ms 5.17 ms
12427f4+dirty 1267.15 ms 1271.30 ms 4.15 ms
9a3ca65+dirty 1247.06 ms 1274.58 ms 27.52 ms
c2a4e9b+dirty 1240.10 ms 1239.22 ms -0.88 ms
b1e8712+dirty 1256.02 ms 1265.14 ms 9.12 ms

App size

Revision Plain With Sentry Diff
ad6c299+dirty 2.36 MiB 2.84 MiB 488.85 KiB
5a22220+dirty 2.36 MiB 2.92 MiB 570.21 KiB
0db0c72+dirty 2.36 MiB 2.84 MiB 487.01 KiB
d0bf494+dirty 2.36 MiB 2.83 MiB 481.15 KiB
76d1baf+dirty 2.36 MiB 2.82 MiB 469.45 KiB
2534337+dirty 2.36 MiB 2.88 MiB 525.47 KiB
12427f4+dirty 2.36 MiB 2.88 MiB 530.38 KiB
9a3ca65+dirty 2.36 MiB 2.82 MiB 462.89 KiB
c2a4e9b+dirty 2.36 MiB 3.08 MiB 734.00 KiB
b1e8712+dirty 2.36 MiB 2.84 MiB 488.84 KiB

Previous results on branch: kw-fix-normalize-context-before-native

Startup times

Revision Plain With Sentry Diff
17fbf68+dirty 1234.78 ms 1233.54 ms -1.23 ms

App size

Revision Plain With Sentry Diff
17fbf68+dirty 2.36 MiB 3.14 MiB 793.41 KiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 14, 2024 •
edited
Loading

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 386.83 ms 432.02 ms 45.19 ms
Size 7.15 MiB 8.39 MiB 1.24 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
80b2ce3+dirty 271.29 ms 316.47 ms 45.18 ms
31fcca2+dirty 366.64 ms 395.78 ms 29.14 ms
4a6664f+dirty 357.02 ms 394.91 ms 37.89 ms
27ef4ee+dirty 296.71 ms 351.00 ms 54.29 ms
52a8031+dirty 330.72 ms 358.76 ms 28.03 ms
9433f35+dirty 265.50 ms 336.08 ms 70.58 ms
2534337+dirty 597.14 ms 665.04 ms 67.90 ms
575f9da+dirty 337.15 ms 370.47 ms 33.32 ms
e540498+dirty 408.56 ms 480.00 ms 71.44 ms
b95b8af+dirty 392.94 ms 428.00 ms 35.06 ms

App size

Revision Plain With Sentry Diff
80b2ce3+dirty 7.15 MiB 8.04 MiB 911.02 KiB
31fcca2+dirty 7.15 MiB 8.18 MiB 1.03 MiB
4a6664f+dirty 7.15 MiB 8.22 MiB 1.07 MiB
27ef4ee+dirty 7.15 MiB 8.08 MiB 959.49 KiB
52a8031+dirty 7.15 MiB 8.09 MiB 965.95 KiB
9433f35+dirty 7.15 MiB 8.08 MiB 959.34 KiB
2534337+dirty 7.15 MiB 8.11 MiB 988.68 KiB
575f9da+dirty 7.15 MiB 8.10 MiB 979.68 KiB
e540498+dirty 7.15 MiB 8.38 MiB 1.23 MiB
b95b8af+dirty 7.15 MiB 8.38 MiB 1.23 MiB

Previous results on branch: kw-fix-normalize-context-before-native

Startup times

Revision Plain With Sentry Diff
17fbf68+dirty 395.52 ms 450.73 ms 55.21 ms

App size

Revision Plain With Sentry Diff
17fbf68+dirty 7.15 MiB 8.39 MiB 1.24 MiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 14, 2024 •
edited
Loading

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1230.41 ms 1234.27 ms 3.86 ms
Size 2.92 MiB 3.69 MiB 794.34 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
ad6c299+dirty 1248.50 ms 1248.88 ms 0.38 ms
5a22220+dirty 1246.18 ms 1249.61 ms 3.43 ms
0db0c72+dirty 1258.88 ms 1262.52 ms 3.64 ms
d0bf494+dirty 1266.20 ms 1267.52 ms 1.32 ms
76d1baf+dirty 1245.00 ms 1257.76 ms 12.76 ms
2534337+dirty 1220.87 ms 1221.47 ms 0.60 ms
12427f4+dirty 1224.90 ms 1231.40 ms 6.50 ms
9a3ca65+dirty 1276.40 ms 1279.14 ms 2.74 ms
c2a4e9b+dirty 1247.39 ms 1243.04 ms -4.35 ms
b1e8712+dirty 1284.11 ms 1297.82 ms 13.71 ms

App size

Revision Plain With Sentry Diff
ad6c299+dirty 2.92 MiB 3.40 MiB 494.12 KiB
5a22220+dirty 2.92 MiB 3.48 MiB 575.81 KiB
0db0c72+dirty 2.92 MiB 3.40 MiB 492.71 KiB
d0bf494+dirty 2.92 MiB 3.40 MiB 488.08 KiB
76d1baf+dirty 2.92 MiB 3.38 MiB 475.74 KiB
2534337+dirty 2.92 MiB 3.43 MiB 529.76 KiB
12427f4+dirty 2.92 MiB 3.44 MiB 533.29 KiB
9a3ca65+dirty 2.92 MiB 3.37 MiB 464.32 KiB
c2a4e9b+dirty 2.92 MiB 3.64 MiB 739.91 KiB
b1e8712+dirty 2.92 MiB 3.40 MiB 494.15 KiB

Previous results on branch: kw-fix-normalize-context-before-native

Startup times

Revision Plain With Sentry Diff
17fbf68+dirty 1230.82 ms 1230.27 ms -0.55 ms

App size

Revision Plain With Sentry Diff
17fbf68+dirty 2.92 MiB 3.69 MiB 794.32 KiB

@krystofwoldrich krystofwoldrich changed the title fix: setContext correctly processes not plain JS object fix: setContext correctly processes non-plain JS object Oct 14, 2024
antonis
antonis reviewed Oct 14, 2024
View reviewed changes
src/js/wrapper.ts Outdated Show resolved Hide resolved
antonis
antonis reviewed Oct 14, 2024
View reviewed changes
src/js/wrapper.ts Outdated Show resolved Hide resolved
antonis
antonis reviewed Oct 14, 2024
View reviewed changes
src/js/wrapper.ts Outdated Show resolved Hide resolved
antonis
antonis reviewed Oct 14, 2024
View reviewed changes
src/js/wrapper.ts Outdated Show resolved Hide resolved
antonis
antonis approved these changes Oct 14, 2024
View reviewed changes
Copy link
Collaborator

@antonis antonis left a comment

Choose a reason for hiding this comment

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

LGTM 🎉
Added some NP suggestions.

krystofwoldrich and others added 2 commits October 14, 2024 16:16
@krystofwoldrich @antonis
Apply suggestions from code review
9ed26d0 
Co-authored-by: Antonis Lilis <antonis.lilis@gmail.com>
@krystofwoldrich
Update wrapper.ts
718ad07
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 14, 2024 •
edited
Loading

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 430.36 ms 415.82 ms -14.54 ms
Size 17.73 MiB 20.11 MiB 2.38 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
0677344 327.74 ms 337.14 ms 9.40 ms
52a8031+dirty 311.55 ms 321.37 ms 9.82 ms
22e31b6 396.48 ms 419.64 ms 23.16 ms
52c0562 453.04 ms 434.71 ms -18.33 ms
0ebca77 414.93 ms 444.49 ms 29.56 ms
e73f4ed+dirty 332.96 ms 354.33 ms 21.37 ms
9433f35 347.64 ms 356.22 ms 8.58 ms
148f924 492.65 ms 500.28 ms 7.63 ms
457e29f 398.10 ms 421.39 ms 23.29 ms
1d86dd6 405.14 ms 411.06 ms 5.92 ms

App size

Revision Plain With Sentry Diff
0677344 17.73 MiB 19.81 MiB 2.07 MiB
52a8031+dirty 17.73 MiB 20.04 MiB 2.31 MiB
22e31b6 17.73 MiB 19.84 MiB 2.10 MiB
52c0562 17.73 MiB 20.11 MiB 2.38 MiB
0ebca77 17.73 MiB 19.95 MiB 2.21 MiB
e73f4ed+dirty 17.73 MiB 20.04 MiB 2.31 MiB
9433f35 17.73 MiB 19.81 MiB 2.08 MiB
148f924 17.73 MiB 19.94 MiB 2.21 MiB
457e29f 17.73 MiB 19.84 MiB 2.10 MiB
1d86dd6 17.73 MiB 19.86 MiB 2.12 MiB

Previous results on branch: kw-fix-normalize-context-before-native

Startup times

Revision Plain With Sentry Diff
17fbf68 472.47 ms 455.54 ms -16.93 ms

App size

Revision Plain With Sentry Diff
17fbf68 17.73 MiB 20.11 MiB 2.38 MiB

lucas-zimerman
lucas-zimerman approved these changes Oct 14, 2024
View reviewed changes
Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

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

Looking good, LGTM!

@krystofwoldrich krystofwoldrich enabled auto-merge (squash) October 15, 2024 08:51
@krystofwoldrich krystofwoldrich merged commit ac41368 into main Oct 15, 2024
58 checks passed
@krystofwoldrich krystofwoldrich deleted the kw-fix-normalize-context-before-native branch October 15, 2024 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antonis antonis antonis approved these changes

@lucas-zimerman lucas-zimerman lucas-zimerman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

com.facebook.react.bridge.ReadableNativeArray cannot be cast to com.facebook.react.bridge.ReadableNativeMap
3 participants
@krystofwoldrich @antonis @lucas-zimerman