-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(web-vitals): Add error handling for invalid object keys in WeakMap
#18809
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
Conversation
| // --- START Sentry-custom code (try/catch wrapping) --- | ||
| // Fix for cases where identityObj is not a valid key for WeakMap (sometimes a problem in Safari) | ||
| // Just return a new instance without caching it in instanceMap | ||
| return new ClassObj(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test for fix PR per review rules
Low Severity · Bugbot Rules
Per the review rules: "When reviewing a fix PR, check if the PR includes at least one unit, integration or e2e test that tests the regression this PR fixes." This fix PR adds error handling for invalid WeakMap keys in Safari but the diff contains no accompanying test to verify the fix works correctly. A test that simulates the Safari TypeError scenario would help prevent regressions.
| return instanceMap.get(identityObj)! as T; | ||
| } catch (e) { | ||
| // --- START Sentry-custom code (try/catch wrapping) --- | ||
| // Fix for cases where identityObj is not a valid key for WeakMap (sometimes a problem in Safari) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: Is this issue easily reproducible? If so it'd be nice if we can somehow add a E2E test, if not I'm fine as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, sadly it's not :/
size-limit report 📦
|
Lms24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing! As discussed, it's not ideal that we can't repo this but I think in this case, let's merge and release it and check back with the affected customer if it fixes the issue.
A customer experienced the following issue on iOS Safari 18.6.2:
TypeError: WeakMap keys must be objects or non-registered symbols.The culprit is probably in web vitals
initUniquefunction (which is vendored in).This fix adds a try/catch to handle edge cases where invalid keys are passed to WeakMap, returning a new instance without caching when validation fails.
Closes #18810 (added automatically)