souporserious/react-measure

Error when the measured child is in a CSSTransition

Closed this issue · 10 comments

kmjennison commented

As of v2.2.2, there's an error when the measured component is in a react-transition-group CSSAnimation component:

Uncaught TypeError: Cannot read property 'getBoundingClientRect' of null

It occurs here. I'm guessing it's trying to measure a node after the transition removes it.

This did not throw an error in v2.1.3.

Error in v2.2.2 (note the console logs when you click the button):
https://jsfiddle.net/31a0vLmc/

No error in v2.1.3:
https://jsfiddle.net/yr6qcgmu/

souporserious commented

Hmm this might have been fixed in #125. I’ll cut a new release tomorrow with that fix in it. Thanks for filing an issue! 🙏

souporserious commented

@kmjennison can you please check the latest version v2.2.3 to see if this error is still occurring?

piecyk commented

Had a very quick look, the error is there but it's not a bug... As the v2.x don't work with observing multi elements, and we have this kind of scenario here. Didn't check way it was working with v2.1.3 but there was the bug 😄

@kmjennison proper solution would be to warp every child with it's own measure
https://jsfiddle.net/uch4zk2e/1/

@souporserious What is the status with v3 branch? Maybe we can create some kind of umbrella issue that we sketch the overall design and missing pieces.

On other side React hooks are coming 🎉as the v3 will try to provide nicer api for multi elements measurement it will be hard dealing with sets of refs, see the comment facebook/react#14072 (comment)

  • 👍2
piecyk commented

Regarding my last comment, observing multi elements... Using the ReactTransitionGroup like in the example will not provide any event's from ResizeObserver when the dimension changes... only for initial mount. As you can see in the logs below, ReactTransitionGroup will call ref with null for unmounted component after previous visible was removed, causing this to unobserve current visible ( but still observing initial one ) https://github.com/souporserious/react-measure/blob/master/src/with-content-rect.js#L79

Bug is happening because

  1. on initial render we set ref to hi, everything is good we observe this._resizeObserver.observe(node) set _node = hi ref
  2. click is happen
  3. set ref to by, ref is not null soo we call again this._resizeObserver.observe(node) and set _node to by ref... and here it breaks as react-measure relays that before setting new _node we unobserve old one
  4. hi is unmounted, call ref with null, react-measure oo ref null, unobserve by, _node = null
    • in some situation the ResizeObserver kiks in as we didn't unobserve intial hi, _node is null, throw 'getBoundingClientRect' of null
// initial render 
this.state.sayHi: true
set hi ref <div style=​"width:​ 50px;​ height:​ 30px;​">​Hello!​</div>​

// next render, click 
this.state.sayHi: false
set by ref <div style=​"width:​ 100px;​ height:​ 30px;​" class=​"example-enter example-enter-active">​Goodbye​</div>​
set hi ref null

// next render, click 
this.state.sayHi: true
set hi ref <div style=​"width:​ 50px;​ height:​ 30px;​" class=​"example-enter example-enter-active">​Hello!​</div>​
set by ref null

It was working in v2.1.3 because disconnect being used instead of unobserve for ResizeObserver,
basic it was doing Unhooks this observer from all targets specified in previous observe() calls This was cleaning previous targets to don't be called.

IMHO this is not a bug in react-measure.

// cc @kmjennison

kmjennison commented

Thanks @piecyk, that makes sense.

Is there a way for react-measure to ensure that it's only measuring one element at a time? If so, it would be helpful to log a warning when the user tries to measure more than one.

souporserious commented

@piecyk I've thought about hooks a lot with react-measure. Going to work on a new release now that there's an official date when they will launch. I'll most likely move to a v4 branch with a hooks first API in mind with a render function component as an alternative. Also, thank you for the explanation on measuring multiple elements 🙏.

@kmjennison the current API doesn't lend to measuring multiple elements easily right now. It will definitely be first class in the new API I'm working on. Hooks should make reasoning about this a lot simpler.

  • 👍1
piecyk commented

@kmjennison we can tweak the _handleRef method to always unobserve prev node, before starting to observe next one. Something like this piecyk@8f542d3 ping @souporserious if i should submit PR.

@souporserious Great to hear that, ping me if help is needed 💪 Will also try investigate some API proposals.

  • 👍1
souporserious commented

@piecyk love that idea for _handleRef! Please feel free to submit a PR 🙏.

piecyk commented

@kmjennison imho you can close the issue now as the PR was merged, thanks!

  • 👍1
souporserious commented

Thanks, @piecyk! @kmjennison please feel free to reopen if you experience any other issues.