Skip to content
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

ResizeObserver cannot be initialized if document.body doesn't exist yet #1311

Closed
AngusMorton opened this issue May 22, 2024 · 5 comments
Closed
Labels
type:defect Bugs or weaknesses. The issue has to contain steps to reproduce.

Comments

@AngusMorton
Copy link

We stream our page in and initialize Snowplow in the <head>, so it's possible for the tracker to start before the <body> has been sent if the <body> doesn't exist when Snowplow initializes it will throw an exception because document.body doesn't exist when it tries to set up the ResizeObserver.

image

TypeError: Failed to execute 'observe' on 'ResizeObserver': parameter 1 is not of type 'Element'.

Specifically, the issue is here where we assume document.body is defined.

Possible Fix
Maybe we should use the resize event instead of a ResizeObserver. It has better support on old browsers and is potentially closer to what we want to know (when the window innerHeight and innerWidth have changed), but we may need to implement some kind of debounce.

Alternatively, we could just guard the initializeResizeObserver code with an if (document.body).

I'm happy to contribute either of those changes.

Workaround
Using <script defer src="..."> instead of <script async src="..."> works around the issue because the script gets evaluated after the DOM is loaded.

Snowplow Version
3.23.0

@AngusMorton AngusMorton added the type:defect Bugs or weaknesses. The issue has to contain steps to reproduce. label May 22, 2024
@matus-tomlein
Copy link
Contributor

Hi @AngusMorton, thanks for the report and sorry for getting back to you late!

Using the resize event instead of ResizeObserver does make sense to me and could also fix issue #1306 that we haven't been able to reproduce so far. I can't think of a reason not to use the resize event over the ResizeObserver and as you mentioned it is compatible with more browsers.

Adding debouncing might be a good idea too to keep the performance too.

If you're happy to implement this change, we would really appreciate the contribution!

@EricHeitmuller-Gusto
Copy link
Contributor

Sent a potential stopgap PR until such time as this can be changed to use the resize event.

@EricHeitmuller-Gusto
Copy link
Contributor

Hey folks, would it be possible to do a patch release for the stopgap change I've contributed? We're currently blocked on it.

@matus-tomlein
Copy link
Contributor

Sorry for the delay @EricHeitmuller-Gusto, we'll aim to release it tomorrow!

@EricHeitmuller-Gusto
Copy link
Contributor

@matus-tomlein no apologies necessary - just needed something to tell my team 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:defect Bugs or weaknesses. The issue has to contain steps to reproduce.
Projects
None yet
Development

No branches or pull requests

3 participants