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(promise): Warn users about multiple versions of promise package which can cause unexpected behaviore #3162

Merged
merged 4 commits into from
Jul 17, 2023
Merged

fix(promise): Warn users about multiple versions of promise package which can cause unexpected behaviore #3162

merged 4 commits into from
Jul 17, 2023

Conversation

krystofwoldrich
Copy link
Member

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

We are patching the global promise which might be an unexpected side effect, but we decided to do this to ensure rejected promise tracking is working 100%. Since the promise package is not our dependency but react-native it can happen that some other package or the user installs a different version of the package. We can't fix which version will be used in our integration unless we will change from mode 100% tracking working to 100% using the react-native version but it might cause the tracking not to work.

Therefore I've added checks and guidance for users that run into this issue. As the solution is simply to install the exact same version as RN uses as direct dep to the app.

💡 Motivation and Context

💚 How did you test it?

Repro from #3160
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

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2023 •
edited
Loading

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 345.79 ms 349.10 ms 3.31 ms
Size 17.73 MiB 19.82 MiB 2.08 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
52a8031+dirty 311.55 ms 321.37 ms 9.82 ms
acadc0f+dirty 373.24 ms 381.51 ms 8.27 ms
15c80ab+dirty 336.27 ms 350.58 ms 14.31 ms
3853f43 329.68 ms 346.32 ms 16.64 ms
e2b64fe 316.88 ms 330.23 ms 13.35 ms
ad6c299 375.94 ms 382.02 ms 6.08 ms
34aba08 328.10 ms 342.84 ms 14.74 ms
d0bf494+dirty 375.37 ms 395.14 ms 19.77 ms
0db0c72 372.12 ms 386.00 ms 13.88 ms
d197b5c+dirty 338.94 ms 354.87 ms 15.93 ms

App size

Revision Plain With Sentry Diff
52a8031+dirty 17.73 MiB 20.04 MiB 2.31 MiB
acadc0f+dirty 17.73 MiB 19.75 MiB 2.01 MiB
15c80ab+dirty 17.73 MiB 20.04 MiB 2.31 MiB
3853f43 17.73 MiB 19.81 MiB 2.08 MiB
e2b64fe 17.73 MiB 19.80 MiB 2.07 MiB
ad6c299 17.73 MiB 19.75 MiB 2.02 MiB
34aba08 17.73 MiB 19.80 MiB 2.07 MiB
d0bf494+dirty 17.73 MiB 19.75 MiB 2.02 MiB
0db0c72 17.73 MiB 19.75 MiB 2.02 MiB
d197b5c+dirty 17.73 MiB 20.04 MiB 2.31 MiB

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2023

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1266.94 ms 1269.78 ms 2.84 ms
Size 2.92 MiB 3.41 MiB 503.73 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
70caa60+dirty 1279.08 ms 1281.54 ms 2.46 ms
9a3ca65+dirty 1276.40 ms 1279.14 ms 2.74 ms
e2b64fe+dirty 1285.78 ms 1297.56 ms 11.78 ms
d197b5c+dirty 1234.80 ms 1249.20 ms 14.40 ms
15c80ab+dirty 1248.41 ms 1251.24 ms 2.83 ms
0db0c72+dirty 1258.88 ms 1262.52 ms 3.64 ms
b1e8712+dirty 1284.11 ms 1297.82 ms 13.71 ms
8900e1a+dirty 1268.36 ms 1273.04 ms 4.68 ms
76d1baf+dirty 1245.00 ms 1257.76 ms 12.76 ms
52a8031+dirty 1255.96 ms 1273.00 ms 17.04 ms

App size

Revision Plain With Sentry Diff
70caa60+dirty 2.92 MiB 3.39 MiB 486.04 KiB
9a3ca65+dirty 2.92 MiB 3.37 MiB 464.32 KiB
e2b64fe+dirty 2.92 MiB 3.41 MiB 499.97 KiB
d197b5c+dirty 2.92 MiB 3.37 MiB 464.41 KiB
15c80ab+dirty 2.92 MiB 3.39 MiB 481.56 KiB
0db0c72+dirty 2.92 MiB 3.40 MiB 492.71 KiB
b1e8712+dirty 2.92 MiB 3.40 MiB 494.15 KiB
8900e1a+dirty 2.92 MiB 3.39 MiB 485.96 KiB
76d1baf+dirty 2.92 MiB 3.38 MiB 475.74 KiB
52a8031+dirty 2.92 MiB 3.38 MiB 475.71 KiB

lucas-zimerman
lucas-zimerman reviewed Jul 12, 2023
View reviewed changes
src/js/integrations/reactnativeerrorhandlers.ts
// eslint-disable-next-line @typescript-eslint/no-var-requires,import/no-extraneous-dependencies
const Promise = require('promise/setimmediate/es6-extensions');
const PromisePackagePromise = require('promise/setimmediate/es6-extensions');
const UsedPromisePolyfill = this._getPromisePolyfill();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like PromisePackagePromise and UsedPromisePolyfill are basically the same, if they are the same I would like to suggest just using one of them

@github-actions
Copy link
Contributor

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1244.76 ms 1245.00 ms 0.24 ms
Size 2.36 MiB 2.85 MiB 500.02 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
b1e8712+dirty 1256.02 ms 1265.14 ms 9.12 ms
3ffcddd+dirty 1244.47 ms 1264.14 ms 19.67 ms
52a8031+dirty 1280.88 ms 1289.78 ms 8.90 ms
0677344+dirty 1276.70 ms 1300.07 ms 23.37 ms
0db0c72+dirty 1275.02 ms 1285.84 ms 10.82 ms
acadc0f+dirty 1264.38 ms 1290.06 ms 25.68 ms
15c80ab+dirty 1223.74 ms 1228.96 ms 5.22 ms
34aba08+dirty 1276.78 ms 1308.52 ms 31.74 ms
d0bf494+dirty 1289.40 ms 1298.40 ms 9.00 ms
d197b5c+dirty 1217.61 ms 1242.66 ms 25.05 ms

App size

Revision Plain With Sentry Diff
b1e8712+dirty 2.36 MiB 2.84 MiB 488.84 KiB
3ffcddd+dirty 2.36 MiB 2.84 MiB 489.60 KiB
52a8031+dirty 2.36 MiB 2.82 MiB 469.44 KiB
0677344+dirty 2.36 MiB 2.85 MiB 496.81 KiB
0db0c72+dirty 2.36 MiB 2.84 MiB 487.01 KiB
acadc0f+dirty 2.36 MiB 2.83 MiB 480.37 KiB
15c80ab+dirty 2.36 MiB 2.83 MiB 474.49 KiB
34aba08+dirty 2.36 MiB 2.85 MiB 495.32 KiB
d0bf494+dirty 2.36 MiB 2.83 MiB 481.15 KiB
d197b5c+dirty 2.36 MiB 2.82 MiB 462.86 KiB

@github-actions
Copy link
Contributor

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 272.58 ms 317.90 ms 45.32 ms
Size 7.15 MiB 8.08 MiB 959.49 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
b1e8712+dirty 322.55 ms 331.84 ms 9.29 ms
3ffcddd+dirty 244.82 ms 295.47 ms 50.65 ms
52a8031+dirty 330.72 ms 358.76 ms 28.03 ms
0677344+dirty 288.40 ms 391.44 ms 103.04 ms
0db0c72+dirty 335.20 ms 351.06 ms 15.86 ms
acadc0f+dirty 259.04 ms 304.67 ms 45.63 ms
15c80ab+dirty 276.38 ms 327.54 ms 51.17 ms
34aba08+dirty 331.79 ms 376.69 ms 44.91 ms
d0bf494+dirty 253.73 ms 308.23 ms 54.49 ms
d197b5c+dirty 258.75 ms 313.61 ms 54.86 ms

App size

Revision Plain With Sentry Diff
b1e8712+dirty 7.15 MiB 8.04 MiB 912.27 KiB
3ffcddd+dirty 7.15 MiB 8.04 MiB 912.17 KiB
52a8031+dirty 7.15 MiB 8.09 MiB 965.95 KiB
0677344+dirty 7.15 MiB 8.07 MiB 949.80 KiB
0db0c72+dirty 7.15 MiB 8.04 MiB 911.02 KiB
acadc0f+dirty 7.15 MiB 8.03 MiB 903.20 KiB
15c80ab+dirty 7.15 MiB 8.09 MiB 966.13 KiB
34aba08+dirty 7.15 MiB 8.07 MiB 946.13 KiB
d0bf494+dirty 7.15 MiB 8.04 MiB 910.85 KiB
d197b5c+dirty 7.15 MiB 8.09 MiB 962.72 KiB

@krystofwoldrich krystofwoldrich merged commit efbcb37 into main Jul 17, 2023
@krystofwoldrich krystofwoldrich deleted the kw-fix-multiple-promise-packages branch July 17, 2023 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucas-zimerman lucas-zimerman lucas-zimerman left review comments

@marandaneto marandaneto marandaneto 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.

enabled: false breaks Promise.allSettled
3 participants
@krystofwoldrich @marandaneto @lucas-zimerman