Remove visibiliytchange event listeners when no longer required #627
Conversation
|
Could anyone help to approved this PR and publish a new version? Same issue occured and needs to fix. |
|
@leoswing can you explain the urgency? Our understand and external analysis in #622 (comment) suggests the impact is minimal. But yes I'd still like to fix this and release a new version soon. |
@tunetheweb hi, in our scenario，the application of micro-frontend seems encountered the serious memory leak problems. So much appreciated if a fix version could be released soon in these days. Thanks a lot ~ |
|
@tunetheweb happens in our Mattermost app as well |
|
I agree it's happening. What is not so clear is if this is causing problems as per previous comments. So you're saying you upgraded to v5 and started experiencing serious memory leak problems you did not see in v4.2.4.? |
| cb(); | ||
| // Remove the above event listener since no longer required. | ||
| // See: https://github.com/GoogleChrome/web-vitals/issues/622 | ||
| document.removeEventListener('visibilitychange', cb); |
There was a problem hiding this comment.
| document.removeEventListener('visibilitychange', cb); | |
| document.removeEventListener('visibilitychange', cb, {once: true}); |
I think you might need to make the signature exactly the same in order for the listener ti get removed. Can you confirm?
There was a problem hiding this comment.
I just tried and it seems to work both ways, so LGTM.
There was a problem hiding this comment.
removeEventListener doesn't have the once option and when I tried this originally I got an error. Doing it without seemed to work fine.
Bumps the vendored-in web vitals library to include the changes between `5.0.2` <-> `5.1.0` from upstream #### Changes from upstream - Remove `visibilitychange` event listeners when no longer required [#627](GoogleChrome/web-vitals#627) - Register visibility-change early [#637](GoogleChrome/web-vitals#637) - Only finalize LCP on user events (isTrusted=true) [#635](GoogleChrome/web-vitals#635) - Fallback to default getSelector if custom function is null or undefined [#634](GoogleChrome/web-vitals#634) #### Our own Changes - Added `addPageListener` and `removePageListener` utilities because the upstream package changed the listeners to be added on `window` instead of `document`, so I added those utilities to avoid having to check for window every time we try to add a listener.
4 participants
Fixes #622