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

Update nuances of ref callback calling #8333

Closed
wants to merge 1 commit into from
Closed

Update nuances of ref callback calling #8333

wants to merge 1 commit into from

Conversation

unel
Copy link

@unel unel commented Nov 17, 2016

For more transparency

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@lacker
Copy link
Contributor

lacker commented Nov 29, 2016

Hmm, this sentence doesn't quite make sense to me. For every render, React calls the ref callback twice? Not on the first render, right? And what is the function instance exactly that this sentence refers to?

@brigand
Copy link
Contributor

brigand commented Nov 29, 2016

Rough concept:

When react diffs the virtual dom, if the ref prop is not strictly equal (===) to the previous ref callback, it will call the previous ref callback with null before calling the new callback with the dom element. When the component unmounts, the ref callback will be called with null. This allows you to do clean up work, similar to componentWillUnmount.

Maybe some mention of defining ref callbacks in constructor or using the public class fields proposal.

@lacker
Copy link
Contributor

lacker commented Nov 29, 2016

So the right takeaway is basically, you should only use ref callbacks with the ref={(foo) => this.foo = foo} pattern. If you do that, you don't need to worry, you'll just get some weirdo nulls sometimes but whatever. If you don't do that, then you are asking for trouble.

@gaearon
Copy link
Collaborator

gaearon commented Jan 4, 2017

Thanks for the PR. Closing as the author did not respond to review, but happy to reopen if you'd like to reword it as suggested!

@gaearon gaearon closed this Jan 4, 2017
gyfis added a commit to gyfis/react that referenced this pull request 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 pull request 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.
@unel
Copy link
Author

unel commented Jan 10, 2017

@lacker @gaearon

So the right takeaway is basically, you should only use ref callbacks with the ref={(foo) => this.foo = foo} pattern. If you do that, you don't need to worry, you'll just get some weirdo nulls sometimes but whatever. If you don't do that, then you are asking for trouble.

If, we MUST using only this pattern, then it is redundant, because It is much easier to use the ref = "foo" pattern.

In other side, ref-function, allows write link to component with different model, e.g. in observable object. And in this case, it would be nice to know about the "extra" ref calls (for debounce save method or debounce listeners of that observable object)

@unel
Copy link
Author

unel commented Jan 10, 2017

Also I think in situation where described cases when ref calls, most developers thinks that ONLY this cases exists.

@syranide
Copy link
Contributor

If, we MUST using only this pattern, then it is redundant, because It is much easier to use the ref = "foo" pattern.

Being "easier" does not make it right, ref="foo" is a flawed pattern, there are important technical differences between the two. The problem here really is the clunky syntax (ref={(c) => this.foo = c}). If it was just ref={=this.foo} then it would be a non-issue. So that means it's a language/syntax problem and not an API problem, but if someone is using refs and this pattern enough to be significantly bothered by it then it probably indicates a misunderstanding or misuse of React.

@unel
Copy link
Author

unel commented Jan 10, 2017

ref="foo" is a flawed pattern, there are important technical differences between the two.

Hmm.. I thought it was just shortcut for ref={(c) => this.refs.componentName = c}, why it is flawed? What differences make it so?

@gaearon
Copy link
Collaborator

gaearon commented Jan 10, 2017

There are multiple problems with it:

  • It requires that React keeps track of currently rendering component (since it can't guess this). This makes React a bit slower.
  • It doesn't work as most people would expect with the "render callback" pattern (e.g. <DataGrid renderRow={this.renderRow} />) because the ref would get placed on DataGrid for the above reason.
  • It is not composable, i.e. if a library puts a ref on the passed child, the user can't put another ref on it (e.g. Any better way to get refs in ReactTransitionGroup? #8734). Callback refs are perfectly composable.

gaearon pushed a commit that referenced this pull request 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 pull request 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)
@sshymko
Copy link

sshymko commented Feb 2, 2017

It doesn't work as most people would expect with the "render callback" pattern (e.g. <DataGrid renderRow={this.renderRow} />) because the ref would get placed on DataGrid for the above reason.

@gaearon Could you please elaborate more, what's the ref declaration and use in your example?

@gaearon
Copy link
Collaborator

gaearon commented Feb 2, 2017

class MyComponent extends Component {
  renderRow = (index) => {
    // This won't work. Ref will get attached to DataTable rather than MyComponent:
    return <input ref={'input-' + index} />;

    // This would work though! Callback refs are awesome.
    return <input ref={input => this['input-' + index] = input} />;
  }
 
  render() {
    return <DataTable data={this.props.data} renderRow={this.renderRow} />
  }
}

@aij
Copy link

aij commented Sep 5, 2017

@gaearon Is there any reason to avoid callback refs like ref={c => this.refs.componentName = c}?

The current documentation seems to suggest this.refs should be avoided completely, linking to your above comment against string refs as it's justification.

@aij aij unassigned lacker Sep 5, 2017
@TrySound
Copy link
Contributor

@aij I guess because this.refs is reserved for different api (string refs).

@VladislavBaranov
Copy link

  handleSubmit = () => {
    this.props.onFormSubmit({
      id: this.props.id,
      title: this.refs.title.value,
      project: this.refs.project.value,
    });
  };
  render() {
    return (
      <div className='ui centered card'>
        <div className='content'>
          <div className='ui form'>
            <div className='field'>
              <label>Title</label>
              <input type='text' ref='title'
                defaultValue={this.props.title}
              />
            </div>
            <div className='field'>
              <label>Project</label>
              <input type='text' ref='project'
                defaultValue={this.props.project}
              />
            </div>
            <div className='ui two bottom attached buttons'>
              <button  onClick={this.handleSubmit}>
                   Create
              </button>
              <button  onClick={this.props.onFormClose}>
                   Cancel
              </button>
            </div>
          </div>
        </div>
      </div>
    );
  }
};```

@TrySound
Copy link
Contributor

TrySound commented Feb 12, 2018

@VladislavBaranov String refs will be deprecated in 16.4. 16.3 will be released with new createRef api, so I don't recommend to rely on string refs anymore.

@mdsEu
Copy link

mdsEu commented Apr 24, 2019

Why not keep string refs as a syntatic sugar for ref={c => this.refs[stringRef]= c} applied during compilation?

String refs are far more readable than callback refs and react could use more readability oriented features.

@jacmkno
Copy link

jacmkno commented May 2, 2019

I hope I'm not the only one who disagree with the reasons to remove string refs. They are a good feature, if anyone knows the reasons in more depth please comment on this issue I've just created: #15559

@RogerLevy
Copy link

RogerLevy commented Jun 27, 2019

class MyComponent extends Component {
  renderRow = (index) => {
    // This won't work. Ref will get attached to DataTable rather than MyComponent:
    return <input ref={'input-' + index} />;

    // This would work though! Callback refs are awesome.
    return <input ref={input => this['input-' + index] = input} />;
  }
 
  render() {
    return <DataTable data={this.props.data} renderRow={this.renderRow} />
  }
}

Couldn't you just:

this.inputs[index] = <input />;

(New to React and still learning so sorry if I'm ignorant of something here)

@panqibao
Copy link

wewe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet