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

Callback ref is passed null and then the component again #4533

Closed
ide opened this issue Jul 31, 2015 · 28 comments
Closed

Callback ref is passed null and then the component again #4533

ide opened this issue Jul 31, 2015 · 28 comments
Milestone

Comments

@ide
Copy link
Contributor

ide commented Jul 31, 2015

I have a super simple React (Native) component with a static root with a callback ref. When the component is mounted, the callback ref is invoked with the root component. Then when I call setState the callback ref receives null and then the root component again.

class Example extends React.Component {
  constructor(props, context) {
    super(props, context);
    console.log('in the constructor');
  }

  render() {
    return (
      <View
        key="root"
        ref={component => { console.log('got ref', component); }}
        style={this.props.style}
      />
    );
  }

  componentDidMount() {
    console.log('in componentDidMount');
    this.setState({});
  }
}

The console logs read:

in the constructor
got ref R…s.c…s.Constructor {props: Object, context: Object, refs: Object, updater: Object, state: null…}
in componentDidMount
got ref null
got ref R…s.c…s.Constructor {props: Object, context: Object, refs: Object, updater: Object, state: null…}

The null ref is confusing to me since the ref'd view isn't unmounted nor is its parent. I believe this is under 0.14 beta 1 (whatever RN master uses).

@syranide
Copy link
Contributor

Can confirm with ReactDOM 0.14.0-beta1 👍

class Example extends React.Component {
  render() {
    return <div ref={c => { console.log(c); }} />;
  }

  componentDidMount() {
    this.setState({});
  }
}
<div data-reactid=​".0">​</div>​
null
<div data-reactid=​".0">​</div>​

@jimfb
Copy link
Contributor

jimfb commented Jul 31, 2015

Sounds like a bug to me, cc @spicyj just in case this is expected behavior. Marking as 0.14 milestone under the assumption that this is a regression.

@jimfb jimfb added this to the 0.14 milestone Jul 31, 2015
@sophiebits
Copy link
Collaborator

Yes, this seems wrong. refs should only be called when a component is mounted or unmounted.

@sophiebits
Copy link
Collaborator

Never mind, this is actually right after all (and the same as 0.13). The function instance is different so we pass null to the old one and the component to the new one.

@ide
Copy link
Contributor Author

ide commented Jul 31, 2015

@spicyj Whoa, OK. Thanks for looking into it.

@unel
Copy link

unel commented Nov 17, 2016

@spicyj

The function instance is different so we pass null to the old one and the component to the new one.

But... why?.. And why is this not reflected in the documentation? :'(

@gaearon
Copy link
Collaborator

gaearon commented Nov 17, 2016

@unel

But... why?

Because the function instance is different on every render. React doesn't know it's the same function "conceptually". Maybe you were passing callback1 and now pass callback2. So it needs to reset the ref for callback1 (since it might never get called again) and then set the ref for callback2 (since it's the new one).

This could be added to the documentation but I'm not sure why it matters. Can you help me understand? If you just set a field in the ref callback you shouldn't have to think about this.

@unel
Copy link

unel commented Nov 17, 2016

@gaearon
I think it's a very important clarification, because in most cases callback1 and callback2 "conceptually" - same and don't expects this behaviour on each render ("Why is my callback called with null, when this component not unmounted?")

And the second reason - the count of calls. 2 calls for whole child component lifecycle obviously less than 2+2*render calls count... For "heavy" handlers it's may be critical.

@gaearon
Copy link
Collaborator

gaearon commented Nov 17, 2016

2 calls for whole child component lifecycle

I'm not sure what you mean. Ref calls should be very cheap because they typically just set a reference. If there is a bottleneck, it likely won't be there.

I think it's a very important clarification

Please feel free to send a doc PR!

@unel
Copy link

unel commented Nov 17, 2016

@gaearon

Please feel free to send a doc PR!

https://github.com/facebook/react/pull/8333/files =)

@saiki-k
Copy link

saiki-k commented Dec 8, 2016

@gaearon

This could be added to the documentation but I'm not sure why it matters. Can you help me understand? If you just set a field in the ref callback you shouldn't have to think about this.

Please correct me if I am wrong — Whenever a new instance of a component is mounted, in the presence of an old instance; the ref callback of the old component instance is called with null, right?

The problem becomes apparent when you have multiple instances of the same component mounted one after the other, and when some logic inside the component is dependent on the component instance, set through the ref callback. Learned this the hard way... :)

@gaearon
Copy link
Collaborator

gaearon commented Dec 8, 2016

@fatman- Can you show a minimal example demonstrating the problem? As long you don't change the type and key, you should get the same instance in ref right after getting null. So in most cases it is unobservable to any code other than ref callback itself.

@saiki-k
Copy link

saiki-k commented Dec 8, 2016

@gaearon You're right, I do get the same instance; but it took some time to wrap my head around "right after getting null" part.

I noticed this when I came across a piece of code that assigned the component instance to a class variable, say this.myCI for instance, and somewhere inside a click handler there was check to see if the event target is a child of the mounted instance — this.myCI.contains(e.target). When there were multiple instances of the same component, the Cannot read property 'contains' of null error message... took some digging to understand the reason behind it.

The problem is not with refs per se, but in code written without "completely" understanding refs. In this regard, it might be a great idea to have this information somewhere in the docs? :)

gyfis added a commit to gyfis/react that referenced this issue Jan 7, 2017
I want to propose some changes to the Refs and the DOM documentation page. 
- Make it clear that string refs are legacy. It seems that this information got lost during the transition to new docs and only some part stayed the same, which was confusing when first reading the docs.
- Clarify and explain that during render, if the ref callback is provided, it will get called twice, first with `null` and then with the rendered DOM element. Discussed in facebook#4533 and first proposed docs change in PR facebook#8333.

I've also planned on adding an example for passing the refs up the component chain based on something I've needed to solve myself (e.g. you want to connect two dynamic components by line in React, so you need to both use refs and propagate them up the chain), and while it would be great to read up on this in the docs, it may be too specific for this section; I'd be happy to hear any recommendations.
gyfis added a commit to gyfis/react that referenced this issue Jan 7, 2017
I want to propose some changes to the Refs and the DOM documentation page. 
- Make it clear that string refs are legacy. It seems that this information got lost during the transition to new docs and only some part stayed the same, which was confusing when first reading the docs.
- Clarify and explain that during render, if the ref callback is provided, it will get called twice, first with `null` and then with the rendered DOM element. Discussed in facebook#4533 and first proposed docs change in PR facebook#8333.

I've also planned on adding an example for passing the refs up the component chain based on something I've needed to solve myself (e.g. you want to connect two dynamic components by line in React, so you need to both use refs and propagate them up the chain), and while it would be great to read up on this in the docs, it may be too specific for this section; I'd be happy to hear any recommendations.
gaearon pushed a commit that referenced this issue Jan 10, 2017
* Update refs-and-the-dom.md

I want to propose some changes to the Refs and the DOM documentation page. 
- Make it clear that string refs are legacy. It seems that this information got lost during the transition to new docs and only some part stayed the same, which was confusing when first reading the docs.
- Clarify and explain that during render, if the ref callback is provided, it will get called twice, first with `null` and then with the rendered DOM element. Discussed in #4533 and first proposed docs change in PR #8333.

I've also planned on adding an example for passing the refs up the component chain based on something I've needed to solve myself (e.g. you want to connect two dynamic components by line in React, so you need to both use refs and propagate them up the chain), and while it would be great to read up on this in the docs, it may be too specific for this section; I'd be happy to hear any recommendations.

* Adds more specific information about the callback

* Moved the ref callback description to the Caveats section

* Fixed suggested nits

* Replace 'each render pass' with 'updates'

* Tweak the wording
gaearon pushed a commit that referenced this issue Jan 12, 2017
* Update refs-and-the-dom.md

I want to propose some changes to the Refs and the DOM documentation page.
- Make it clear that string refs are legacy. It seems that this information got lost during the transition to new docs and only some part stayed the same, which was confusing when first reading the docs.
- Clarify and explain that during render, if the ref callback is provided, it will get called twice, first with `null` and then with the rendered DOM element. Discussed in #4533 and first proposed docs change in PR #8333.

I've also planned on adding an example for passing the refs up the component chain based on something I've needed to solve myself (e.g. you want to connect two dynamic components by line in React, so you need to both use refs and propagate them up the chain), and while it would be great to read up on this in the docs, it may be too specific for this section; I'd be happy to hear any recommendations.

* Adds more specific information about the callback

* Moved the ref callback description to the Caveats section

* Fixed suggested nits

* Replace 'each render pass' with 'updates'

* Tweak the wording

(cherry picked from commit 4a7e06b)
@windsome
Copy link

How deal with the following case?
I want to listen to 'transitionend' of a dom element, I add event listener it in the ref callback currently. what's the right way?

 <div id="myCarousel" className="carousel slide" 
ref={(ele) =>{
             if (this.carousel) {
                 this.carousel.removeEventListener("transitionend", this.stopAnimation.bind(this));
             }
             this.carousel = ele;
             if (this.carousel) {
                 this.carousel.addEventListener("transitionend", this.stopAnimation.bind(this), false);
             }
}} 
onMouseOver={()=>this.enableAnimation(false)} 
onMouseOut={()=>this.enableAnimation(true)}>

@syranide
Copy link
Contributor

As far as I see it, if the node is added/removed sometimes then your code actually does what it should (note however that when you supply the callback inline you will be adding and removing listeners every render, more on this in the docs). One could argue that a better abstraction would be to extract that part of the code out into its own component, but I wouldn't necessary call it wrong to do it that way.

If the node is static and never added/removed then you would probably do best in moving the listener-logic out of the ref and into componentDidMount instead.

@okeydoke
Copy link

okeydoke commented Feb 1, 2017

I'm curious when the documentation says You can avoid this by defining the ref callback as a bound method on the class

Here's a contrived example component,

class Carousel extends Component {
  constructor(props) {
    super(props);
    // if using a 'bound method' is this needed?
    this.refHandler = this.refHandler.bind(this);
  }

  elementsInView() {
    // currently getting a lot of errors where this.containerRefAnonymous would be null
    const elements = this.containerRefAnonymous .querySelectorAll(`[data-inview]`); 
    // do stuff with the elements
  }

  componentDidMount() {
    this.scrollHandler = debounce(this.elementInView, 75).bind(this);
    window.addEventListener('scroll', this.scrollHandler);
  }

  componentWillUnmount() {
    window.removeEventListener('scroll', this.scrollHandler);
  }

  refHandler(domElement) {
    this.containerRef = domElement;
  }

  render() {
    return (
      <div ref={this.refHandler}> // bound method never gets called with null?
        <div ref={c => this.containerRefAnonymous = c;} // can be called with null
          {this.props.children}
        </div>
      </div>
    );
  }
} 

So assuming I replace any instances in our code like c => this.containerRefAnonymous = c; with the this.refHandle approach

a) does the this.refHandler need to be bound to this in the constructor like other event handlers?
b) will the this.refHandler style ever get called with null?

EDIT: To clarify our situation abit:
I've mixed the two approaches in the example but currently we are using ref={c => this.containerRefAnonymous = c;} and getting null references. And instead of wrapping functions like elementsInView with an if statement to check reference isn't null was hoping the 'bound method' approach would never get called with null

@gaearon
Copy link
Collaborator

gaearon commented Feb 1, 2017

@okeydoke Can you create a small example reproducing your problem with nulls? Generally they shouldn't be a problem. The fields shouldn't be null by the time arbitrary event handlers run.

The reason class field avoids this problem is because the function reference to the ref is constant. In this case React doesn’t need to detach and reattach it on every update. But it will be called with null on unmounting anyway.

I think the problem with your example is your debounced handler still runs after unmounting. So far I think it has nothing to do with the nulls during updates. You are unsubscribing in componentWillUnmount but not cancelling the debounce, so the last debounce will fire after unmounting. You should either cancel the debounce, or check for ref’s existence in the method (since you know it can be called after unmounting).

@okeydoke
Copy link

okeydoke commented Feb 1, 2017

@gaearon ah you make a very good point about the debounced calls! I think we have only seen it on components that use a debounced call and it make sense why now you've point it out.

But I'm curious with a bound method would the perf be slightly better since it doesn't have to detach and reattach plus you would have referential equality when using shouldComponentUpdate ?

Thanks for pointing out the debounced though!

@gaearon
Copy link
Collaborator

gaearon commented Feb 2, 2017

It might be slightly better but I wouldn't assume it's a problem in most cases.

@Fadeoc
Copy link

Fadeoc commented Aug 21, 2017

I met this problem on Friday, can not believe this issue already had been attached bug tag is still there after a year.

I need to create certain amount of UL and make them as a auto-slider after every state update, so that I need ref to get all the UL and then change the visibility etc.

But turned out it only works after every componentDidMount, while if there are 4 UL then the ref will return an array with 4 children, and return an array with 8 children (the first 4 are null, perfect matchs what fatman talked about above) after every state update, even I reset the ref to [] in the method before every re-render. I debugged it for two days, had tried put my animation methods into different life cycles, none help. Then thank god I found this issue, then with a filter from null values all work at once.

Maybe I am wrong with my way of using React, I am just a new user, but sometimes I really prefer to use refs, for instance, you need to send a request from the child component to the server and waiting for the response received in the parent component, the child component will re-render after data received as it receive new props, it will then mess up the child's own state from updating in the expecting way, by sharing the same componentDidUpdate, certainly shouldComponentUpdate will take care, then you need to think carefully how to define the return value in shouldComponentUpdate to keep the child component re-rendering by it's own state updating, while avoid that from new parent props.

@gaearon
Copy link
Collaborator

gaearon commented Aug 21, 2017

can not believe this issue already had been attached bug tag is still there after a year.

As it was said above this is not a bug (despite the mistaken label).

sometimes I really prefer to use refs, for instance, you need to send a request from the child component to the server and waiting for the response received in the parent component, the child component will re-render after data received as it receive new props, it will then mess up the child's own state from updating in the expecting way

It is quite hard to understand what you're saying without an example. But my intuition is you're using refs for data flow which is not the intended usage. Please check the documentation (Lifting State Up, Thinking in React) for an explanation of React data flow.

@Fadeoc
Copy link

Fadeoc commented Aug 21, 2017

Thanks for your reply @gaearon.

I was trying to build a child component shows 6 persons' names which are the first 6 names from certain amount of person names, after 3 seconds the page turns to show the next 6 names, and so on until it shows out the last couple of (1-6) names, depending on how many names received in the parent component, then start over again.

To make this, I have a splitPerson method to split all persons into different UL (pages) by 6 a group, for instance, if I receive 100 person names, then there will be 17 UL (pages). In this method, it returns the UL elements.

constructor(props){...};
...
splitPerson() {
  const personNameLength = this.state.persons.length;
  if (personNameLength <= 6) {
    this.pageCount = 1;
  } else {
    this.pageCount = parseInt(personNameLength / 6);
    if (personNameLength % 6 !== 0) {
      this.pageCount += 1;
    }
  }
  this.pageRefs = [];
  this.pages =  [...Array(this.pageCount)].map((pageIndex, i) {
    return <ul ref={ul => this.pageRefs.push(ul)} key={...}>{names}</ul>
  }
}

Then another turnPage method, use refs "this.pageRefs" to get all of these UL and iterate across them every 3 seconds and reset their classes between hidden and visible.

turnPage() {
  if (this.pageCount > 1) {
    this.turnPageInterval = setInterval(() => {
        this.pageRefs.forEach((ul, i) => {
          if (commonModule.hasClass(ul, style.pageVisible)) {
            commonModule.removeClass(ul, style.pageVisible);
            commonModule.addClass(ul, style.pageHidden);
            this.index = i;
          }
        });
        let nextPage;
        if (this.index + 1 >= this.pageCount) {
          nextPage = this.pageRefs[0];
        } else {
          nextPage = this.pageRefs[this.index + 1];
        }
        commonModule.removeClass(nextPage, style.pageHidden);
    }, 3000);
  }
}

Then render static part by render.

  render() {
    this.splitPerson();
    return (
      <div>
        {this.pages}
      </div>
    );

Then animate after mount or update:

  componentDidMount() {
    this.index = 0;
    this.turnPage();
  }
  componentWillUnmount() {
    if (this.turnPageInterval) {
      clearInterval(this.turnPageInterval);
    }
  }
  componentWillReceiveProps(nextProps) {
    if (this.props.persons !== nextProps.persons) {
      this.setState({
        persons: nextProps.persons
      });
    }
  }
  componentWillUpdate() {
    if (this.turnPageInterval) {
      clearInterval(this.turnPageInterval);
    }
  }
  componentDidUpdate() {
    this.turnPage();
  }

Maybe I am doing it the wrong way, but every time I get double times pages after every update, half of them are null refs.

Sorry to take this problem here, now it looks more like a problem should be shown on StackOverflow.

@gaearon
Copy link
Collaborator

gaearon commented Aug 21, 2017

I would recommend to try to replace DOM manipulations through refs with normal React rendering when possible, or using React animation libraries like TransitionGroup or React Motion.

But if you want to keep using refs for this, you just need to change the ref function. Instead of pushing to an array, you can put them into an object by key, like this.pageRefs[key] = ul. Then you'll always have values there.

But again, so much DOM manipulation generally seems suspicious and is often unnecessary.

@Fadeoc
Copy link

Fadeoc commented Aug 21, 2017

Thanks for your time. Yes, I don't feel good with using React this way either.

I was trying to set a state represents the pageIndex, and update it to pageIndex + 1 by this.setState in a
setTimeout, but I need to call this setTimeout in componentDidUpdate.

While at the same time I need to receive new data via props from its parent, that will mess up the state updating progress as the new props will call componentDidUpdate also thus call that setTimeout again.

This problem, the pattern that props changing call componentDidUpdate mess up with child's own state updating, really bugs me for two weeks. Anyway it should be this way, the props changing should update the child, because React is designed this way.

But I do not know what else I can do.

I'll head to Stack Overflow in case wasting your more time.

Thank you.

@gaearon
Copy link
Collaborator

gaearon commented Aug 21, 2017

It looks to me like using TransitionGroup is the best option for what you're trying to do.

https://github.com/reactjs/react-transition-group

marrobins added a commit to marrobins/BotFramework-WebChat that referenced this issue Nov 13, 2017
As per this issue [0], ref callbacks need to handle null arguments. During a re-render, the ref callback from the old component will be passed `null` and the ref callback from the new component instance will be passed the new ref instance. 
[0] facebook/react#4533
compulim pushed a commit to microsoft/BotFramework-WebChat that referenced this issue Dec 5, 2017
As per this issue [0], ref callbacks need to handle null arguments. During a re-render, the ref callback from the old component will be passed `null` and the ref callback from the new component instance will be passed the new ref instance. 
[0] facebook/react#4533
amazingwebdev added a commit to amazingwebdev/react-webchat that referenced this issue Feb 13, 2018
As per this issue [0], ref callbacks need to handle null arguments. During a re-render, the ref callback from the old component will be passed `null` and the ref callback from the new component instance will be passed the new ref instance. 
[0] facebook/react#4533
lilfolr added a commit to React9k/react-timeline-9000 that referenced this issue Jul 24, 2018
@trusktr
Copy link

trusktr commented Aug 28, 2018

Never mind, this is actually right after all (and the same as 0.13). The function instance is different so we pass null to the old one and the component to the new one.

I didn't read the whole thing, but I was just curious: which JS engine leaked from not passing null? Or is that not why null is passed?

@trusktr
Copy link

trusktr commented Aug 28, 2018

For reference, this is a problem in cases like:

class Foo extends React.Component {
  whenWeHaveTheDiv = (div) => {
    div.querySelector('.foo') // do something with it.
  }

  render() { return <div ref={this.whenWeHaveTheDiv}>...</div>}
}

It's simply not intuitive for that to be called more than once per reference. And if we really need to clean up after touching DOM, there's componentWillUnmount.

@gaearon
Copy link
Collaborator

gaearon commented Aug 28, 2018

@trusktr

I didn't read the whole thing, but I was just curious: which JS engine leaked from not passing null? Or is that not why null is passed?

I believe you already filed two issues about this more than a year ago (#9328 and #9574). I responded to you with a long form explanation to this exact question in #9328 (comment). I don’t have more to add to what I already said there.

The code example in your last comment doesn’t seem like it needs to use a ref callback at all. You can use object refs introduced in React 16.3 and a lifecycle hook.

class Foo extends React.Component {
  divRef = React.createRef();
  componentDidMount() {
    const div = this.divRef.current;
    div.querySelector('.foo') // do something with it.
  }
  componentDidUpdate() {
    const div = this.divRef.current;
    div.querySelector('.foo') // do something with it.
  }
  render() { return <div ref={this.divRef}>...</div>}
}

Note this would only work if element with a ref is returned unconditionally from render. Otherwise you'd still have to use null checks — the ref unmounts, after all!

Hope this helps.

@facebook facebook locked as resolved and limited conversation to collaborators Aug 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests