-
Notifications
You must be signed in to change notification settings - Fork 24
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
Mute "ResizeObserver loop limit exceeded" exception #2032
Conversation
f78e857
to
f46a2cf
Compare
this.resizeObserverService.observe(this.elementRef, () => { | ||
this.scaleHeader(); | ||
}); | ||
} catch (e) {} |
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.
Nice!
I think we should check the message field of the exception as to not unintentionally mute other exceptions.
The standard error message is "ResizeObserver loop completed with undelivered notifications." according to the spec. According to this guy Chromium based browser might have another message set tho for some reason.
Which i believe is the one you might be seeing when testing from an Android based phone.
So we might have to check both.
So something like the below i guess 🤷
catch(e) {
if message is either chrome message or standard message:
do nothing
else:
throw(e);
}
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.
Also if this turns out to give problems or not work - let's revert to the requestAnimationFrame solution. But i think this one might be the least intrusive solution for now.
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.
Reverted to the requestAnimationFrame due to time constraints.
The new proposed solution did not catch the error, and we need to investigate the error object a bit more to find out how to filter it. Unfortunately the process is slow, as we have to build kirby, install it in a project, and run the emulator to get feedback, so we will have to investigate more at a later time.
f46a2cf
to
44a922c
Compare
44a922c
to
0acdca5
Compare
0acdca5
to
f78e857
Compare
f78e857
to
ad39f2d
Compare
ad39f2d
to
3cb01b7
Compare
libs/designsystem/src/lib/directives/fit-heading/fit-heading.directive.ts
Outdated
Show resolved
Hide resolved
This is possibly less performant, according to https://stackoverflow.com/questions/49384120/resizeobserver-loop-limit-exceeded but seems to be the solution that is able to suppress the error in our use case.
This reverts commit 912e97b.
Which issue does this PR close?
This PR closes #2031
What is the new behavior?
No ResizeObserver exceptions thrown in Azure App Insight when navigating to select pages
Other routes should be thoroughly tested.
Does this PR introduce a breaking change?
It's unclear if the changes has any sideeffects. It should be tested.
Are there any additional context?
Checklist:
The following tasks should be carried out in sequence in order to follow the process of contributing correctly.
Reminders
Review
When the pull request has been approved it will be automatically merged to master via automerge.