Skip to content

fix(sdk): Sentry.wrap doesn't enforce any keys on the wrapped component props #3332

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
Merged
merged 3 commits into from
Oct 9, 2023
Merged

fix(sdk): Sentry.wrap doesn't enforce any keys on the wrapped component props #3332

merged 3 commits into from
Oct 9, 2023

Conversation

@krystofwoldrich
Copy link
Contributor

@krystofwoldrich krystofwoldrich commented Oct 9, 2023

📢 Type of change

  • Bugfix

📜 Description

closes: #3311
adds test to prevent this regression in the future

💚 How did you test it?

unit test, 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 Oct 9, 2023

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 410.65 ms 449.15 ms 38.50 ms
Size 17.73 MiB 19.84 MiB 2.10 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3ffcddd 302.92 ms 315.80 ms 12.88 ms
3853f43 329.68 ms 346.32 ms 16.64 ms
0db0c72 372.12 ms 386.00 ms 13.88 ms
d197b5c+dirty 338.94 ms 354.87 ms 15.93 ms
abb7058 370.27 ms 389.58 ms 19.31 ms
76d1baf+dirty 335.72 ms 355.52 ms 19.80 ms
0677344 327.74 ms 337.14 ms 9.40 ms
9c48b2c 349.24 ms 385.96 ms 36.72 ms
457e29f 398.10 ms 421.39 ms 23.29 ms
8900e1a+dirty 430.68 ms 456.13 ms 25.44 ms

App size

Revision Plain With Sentry Diff
3ffcddd 17.73 MiB 19.75 MiB 2.02 MiB
3853f43 17.73 MiB 19.81 MiB 2.08 MiB
0db0c72 17.73 MiB 19.75 MiB 2.02 MiB
d197b5c+dirty 17.73 MiB 20.04 MiB 2.31 MiB
abb7058 17.73 MiB 19.83 MiB 2.10 MiB
76d1baf+dirty 17.73 MiB 20.04 MiB 2.31 MiB
0677344 17.73 MiB 19.81 MiB 2.07 MiB
9c48b2c 17.73 MiB 19.80 MiB 2.07 MiB
457e29f 17.73 MiB 19.84 MiB 2.10 MiB
8900e1a+dirty 17.73 MiB 19.75 MiB 2.01 MiB

lforst
lforst reviewed Oct 9, 2023
View reviewed changes
src/js/sdk.tsx
// Deprecated in https://github.com/DefinitelyTyped/DefinitelyTyped/commit/f1b25591890978a92c610ce575ea2ba2bbde6a89
// eslint-disable-next-line deprecation/deprecation
export function wrap<P extends JSX.IntrinsicAttributes>(
export function wrap<P extends Record<string, unknown>>(
Copy link

@lforst lforst Oct 9, 2023

Choose a reason for hiding this comment

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

Hm, we probably want to infer the return type. Let me get back to this. In an hour or so.

Copy link
Contributor Author

@krystofwoldrich krystofwoldrich Oct 9, 2023

Choose a reason for hiding this comment

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

I thought about that too, but so far no one complained about that. But the JSX.IntrinsicAttributes forced users to specify a key attribute in the props.

Copy link

@lforst lforst Oct 9, 2023

Choose a reason for hiding this comment

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

Fair. This is infering the return type anyhow I think 👍

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023 •
edited
Loading

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1295.36 ms 1300.68 ms 5.32 ms
Size 2.36 MiB 2.88 MiB 530.01 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
27ef4ee+dirty 1293.52 ms 1296.08 ms 2.56 ms
0677344+dirty 1276.70 ms 1300.07 ms 23.37 ms
3853f43+dirty 1221.82 ms 1242.64 ms 20.82 ms
ad6c299+dirty 1244.76 ms 1260.10 ms 15.34 ms
34aba08+dirty 1276.78 ms 1308.52 ms 31.74 ms
d197b5c+dirty 1217.61 ms 1242.66 ms 25.05 ms
9433f35+dirty 1246.94 ms 1271.45 ms 24.52 ms
76d1baf+dirty 1244.10 ms 1268.52 ms 24.42 ms
80b2ce3+dirty 1265.92 ms 1268.60 ms 2.69 ms
457e29f+dirty 1253.94 ms 1269.18 ms 15.24 ms

App size

Revision Plain With Sentry Diff
27ef4ee+dirty 2.36 MiB 2.85 MiB 500.03 KiB
0677344+dirty 2.36 MiB 2.85 MiB 496.81 KiB
3853f43+dirty 2.36 MiB 2.85 MiB 499.81 KiB
ad6c299+dirty 2.36 MiB 2.84 MiB 488.85 KiB
34aba08+dirty 2.36 MiB 2.85 MiB 495.32 KiB
d197b5c+dirty 2.36 MiB 2.82 MiB 462.86 KiB
9433f35+dirty 2.36 MiB 2.85 MiB 499.80 KiB
76d1baf+dirty 2.36 MiB 2.82 MiB 469.45 KiB
80b2ce3+dirty 2.36 MiB 2.84 MiB 486.98 KiB
457e29f+dirty 2.36 MiB 2.87 MiB 520.67 KiB

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023 •
edited
Loading

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1271.96 ms 1275.16 ms 3.20 ms
Size 2.92 MiB 3.44 MiB 534.07 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
27ef4ee+dirty 1236.41 ms 1244.90 ms 8.49 ms
0677344+dirty 1252.52 ms 1254.08 ms 1.56 ms
3853f43+dirty 1271.74 ms 1278.04 ms 6.30 ms
ad6c299+dirty 1248.50 ms 1248.88 ms 0.38 ms
34aba08+dirty 1268.58 ms 1276.80 ms 8.22 ms
d197b5c+dirty 1234.80 ms 1249.20 ms 14.40 ms
9433f35+dirty 1232.24 ms 1232.74 ms 0.50 ms
76d1baf+dirty 1245.00 ms 1257.76 ms 12.76 ms
80b2ce3+dirty 1245.12 ms 1262.04 ms 16.92 ms
457e29f+dirty 1256.71 ms 1258.50 ms 1.79 ms

App size

Revision Plain With Sentry Diff
27ef4ee+dirty 2.92 MiB 3.41 MiB 503.72 KiB
0677344+dirty 2.92 MiB 3.41 MiB 500.94 KiB
3853f43+dirty 2.92 MiB 3.41 MiB 503.54 KiB
ad6c299+dirty 2.92 MiB 3.40 MiB 494.12 KiB
34aba08+dirty 2.92 MiB 3.41 MiB 499.03 KiB
d197b5c+dirty 2.92 MiB 3.37 MiB 464.41 KiB
9433f35+dirty 2.92 MiB 3.41 MiB 503.55 KiB
76d1baf+dirty 2.92 MiB 3.38 MiB 475.74 KiB
80b2ce3+dirty 2.92 MiB 3.40 MiB 492.75 KiB
457e29f+dirty 2.92 MiB 3.43 MiB 524.75 KiB

@krystofwoldrich krystofwoldrich enabled auto-merge (squash) October 9, 2023 14:45
@krystofwoldrich krystofwoldrich merged commit ae673b2 into main Oct 9, 2023
@krystofwoldrich krystofwoldrich deleted the kw-fix-wrap-props branch October 9, 2023 14:47
@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 332.26 ms 359.59 ms 27.34 ms
Size 7.15 MiB 8.12 MiB 998.92 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
27ef4ee+dirty 296.71 ms 351.00 ms 54.29 ms
0677344+dirty 288.40 ms 391.44 ms 103.04 ms
3853f43+dirty 278.12 ms 338.72 ms 60.60 ms
ad6c299+dirty 336.47 ms 362.89 ms 26.42 ms
34aba08+dirty 331.79 ms 376.69 ms 44.91 ms
d197b5c+dirty 258.75 ms 313.61 ms 54.86 ms
9433f35+dirty 265.50 ms 336.08 ms 70.58 ms
76d1baf+dirty 339.02 ms 408.65 ms 69.63 ms
80b2ce3+dirty 271.29 ms 316.47 ms 45.18 ms
457e29f+dirty 591.49 ms 612.96 ms 21.47 ms

App size

Revision Plain With Sentry Diff
27ef4ee+dirty 7.15 MiB 8.08 MiB 959.49 KiB
0677344+dirty 7.15 MiB 8.07 MiB 949.80 KiB
3853f43+dirty 7.15 MiB 8.08 MiB 959.34 KiB
ad6c299+dirty 7.15 MiB 8.04 MiB 912.17 KiB
34aba08+dirty 7.15 MiB 8.07 MiB 946.13 KiB
d197b5c+dirty 7.15 MiB 8.09 MiB 962.72 KiB
9433f35+dirty 7.15 MiB 8.08 MiB 959.34 KiB
76d1baf+dirty 7.15 MiB 8.09 MiB 964.41 KiB
80b2ce3+dirty 7.15 MiB 8.04 MiB 911.02 KiB
457e29f+dirty 7.15 MiB 8.10 MiB 981.29 KiB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lucas-zimerman lucas-zimerman Awaiting requested review from lucas-zimerman lucas-zimerman is a code owner

@Lms24 Lms24 Awaiting requested review from Lms24

@AbhiPrasad AbhiPrasad Awaiting requested review from AbhiPrasad

1 more reviewer

@lforst lforst lforst approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

[DEPRECATED] Mobile SDKs
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error IntrinsicAttributes when passing App component to Sentry.wrap

3 participants

@krystofwoldrich @lforst