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

getScrollParents util can push "null" as sole scroll parent when body has not been rendered, causing fatal error #285

Open
circAssimilate opened this issue Apr 24, 2018 · 7 comments
Labels
bug hacktoberfest hacktoberfest issues for newer contributors

Comments

@circAssimilate
Copy link

getScrollParents util can push null as sole scroll parent, causing a fatal error, when the body has not been rendered. This is the line that is leading to the below error:

https://github.com/HubSpot/tether/blob/1fea67260ebdd6bc3d42ebe7f91b367174178d97/src/js/utils.js#L65

Getting Uncaught TypeError: Cannot read property 'addEventListener' of null

In our application, we load our main bundle before the body for performance reasons. Is there a possibility for an option that can delay tethering until the body is ready? Open to any other approaches too.

@FezVrasta
Copy link

Perf reasons? Actually all the best practices say to load js after the body

@circAssimilate
Copy link
Author

I do not agree that it's a best practice to load all JS in the head, but I can understand if that's the requirements for Tether. I could see other users running into this fwiw.

In the case that this is expected, are there any existing options available to defer the creation until an element (or specifically the body element) exists or should we do that on our end before creating a new Tether (e.g. via polling or a Mutation Observer waiting for the body)?

@FezVrasta
Copy link

Nope. It's best practice to load it after the body.

Anyway this library is in maintenance mode, if you need a more up to date library use Popper.js as suggested in the readme

@jamesopti
Copy link

jamesopti commented Apr 26, 2018

Nope. It's best practice to load it after the body.

Theres no such thing as after the body. I think you mean inside the

Actually all the best practices say to load js after the body

This is not true at all. I'll cite Google's recommendation which is focused on blocking scripts, as well as React's getting started documentation as cookie cutter examples of loading JS in the

It's perfectly legitimate to place javascript tags in the to begin fetching them as soon as possible. If a script is cached, its completely possible for it to run before the is defined.

The issue the OP is running into is the Tether constructor being invoked before the has been rendered into the DOM.

At a minimum this behavior should be documented.
More so, the code shouldn't assume that the exists when it runs and handle that case.

@jamesopti
Copy link

Apologies for the snarky tone in my reply!

What I mean to convey is that the author of any javascript bundle should be aware of what dependencies it has on DOM state.

If you choose to load JS before the is defined, you better make sure you wait for or whatever element you're mounting to before calling ReactDOM.render

However because React (like other UI frameworks) allows for components to be created and mounted before being actually inserted into the DOM, any 3rd party library should be clear about it's own dependency on DOM state that is not built into React itself.

Also thanks for the pointer @FezVrasta to this repo being in maintenance. The OP should probably just migrate to Popper as suggested.

@chuckcarpenter
Copy link
Member

chuckcarpenter commented Oct 1, 2019

I can definitely see a clear use case where this wouldn't create a Tether until BODY was available. I'd be open to either a polling function or Mutation Observer option as noted in #285 (comment). Nice solution and suggestion @circAssimilate.

Original location of issue has moved to it's own utils module and is here:

parents.push(el.ownerDocument.body);

@chuckcarpenter chuckcarpenter added the hacktoberfest hacktoberfest issues for newer contributors label Oct 1, 2019
@RobbieTheWagner
Copy link
Member

@chuckcarpenter wouldn't it be better to handle this in the consuming app? Wait until the element you want to tether to exists before creating the tether?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug hacktoberfest hacktoberfest issues for newer contributors
Projects
None yet
Development

No branches or pull requests

5 participants