Avoid undelivered notifications error #1335
Conversation
snowplowcla
added
the
cla:no
|
Thanks for your pull request. Is this your first contribution to a Snowplow open source project? Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://docs.snowplowanalytics.com/docs/contributing/contributor-license-agreement/ to learn more and sign. Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks. |
There was a problem hiding this comment.
LGTM after comments if LGT @matus-tomlein. Look forward to the results of your testing!
We should probably make getBrowserProperties only do the expensive stuff conditionally as well; 1/3 callers don't even use the expensive viewport/documentSize values and it's when the tracker first loads so probably a bad time to be forcing reflow.
| } | ||
| }); | ||
| resizeObserver.observe(document.body); | ||
| resizeObserver.observe(document.documentElement); | ||
| } | ||
|
|
||
| function scheduleBrowserPropertiesCallback(func: () => void) { | ||
| if (requestAnimationFrame) { |
There was a problem hiding this comment.
This needs to check with typeof or against window or it will still throw is not defined (though I'm not sure we offically still support anything that doesn't support rAF anyway).
There was a problem hiding this comment.
I'm not sure we offically still support anything that doesn't support rAF anyway
Assuming the browserslistrc is up to date, it looks like IE 9 is in the supported list of browsers, which doesn't support rAF. I'll remove the fallback, though, since ResizeObserver is less well-supported than rAF, this code path won't be (as far as I can tell) called by any browsers that don't support rAF.
I'm happy to make that change if you're open to it, I can do it in a separate PR. |
AngusMorton
force-pushed
the
am/avoid-undelivered-notifications
branch
from
d7e21a7
to
e9c77be
Compare
|
I signed it! |
|
Confirmed! @AngusMorton has signed the Contributor License Agreement. Thanks so much. |
snowplowcla
added
cla:yes
There was a problem hiding this comment.
Except for the browser compatibility, this looks good to me too!
But would be good to get some validation if this actually helps. I have unfortunately been unable to reproduce the issue thus far, so can't test it out. If you have a way to validate it, please do and let us know!
| // queried. It's possible that the forced synchronous layout causes the ResizeObserver | ||
| // to be fired again, leading to an infinite loop and the "ResizeObserver loop completed | ||
| // with undelivered notifications" error. | ||
| readBrowserPropertiesTask = requestAnimationFrame(() => { |
There was a problem hiding this comment.
Unfortunately the requestAnimationFrame API is not supported on all browsers that the JS tracker v3 supports. Could we check if the API is available in the browser and avoid using it if not?
Maybe closes: #1306
I have yet to validate that this change fixes anything. I plan to test this and see if it reduces the error rate. I will comment once I've tested this in production and confirmed that it resolves the errors.
Based on the ResizeObserver documentation, I suspect that calling
readBrowserProperties()forces a synchronous layout, which then enqueues a new resize observer notification if the body size changes when the layout is re-calculated.