Update nuances of ref callback calling #8333

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
@unel

unel commented Nov 17, 2016

For more transparency

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 17, 2016

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.

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

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 17, 2016

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

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

@lacker

This comment has been minimized.

Show comment
Hide comment
@lacker

lacker Nov 29, 2016

Contributor

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?

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

This comment has been minimized.

Show comment
Hide comment
@brigand

brigand Nov 29, 2016

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@lacker

lacker Nov 29, 2016

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 4, 2017

Member

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!

Member

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

Updated docs about refs and the dom
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 #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

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 facebook#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.
@unel

This comment has been minimized.

Show comment
Hide comment
@unel

unel 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 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

This comment has been minimized.

Show comment
Hide comment
@unel

unel Jan 10, 2017

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

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

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Jan 10, 2017

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.

Contributor

syranide commented Jan 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@unel

unel 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?

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 10, 2017

Member

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. #8734). Callback refs are perfectly composable.
Member

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. #8734). Callback refs are perfectly composable.

gaearon added a commit that referenced this pull request Jan 10, 2017

Added more info about refs in the documentation (#8707)
* 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 added a commit that referenced this pull request Jan 12, 2017

Added more info about refs in the documentation (#8707)
* 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

This comment has been minimized.

Show comment
Hide comment
@sshymko

sshymko 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?

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 2, 2017

Member
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} />
  }
}
Member

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} />
  }
}

@OZhurbenko OZhurbenko referenced this pull request in bigbluebutton/bigbluebutton Jun 4, 2017

Merged

[HTML5] String refs to callback refs #3987

@aij

This comment has been minimized.

Show comment
Hide comment
@aij

aij 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 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

@whns whns referenced this pull request in tomchentw/react-toastr Oct 16, 2017

Closed

Update README - clarify the import example. #130

@Nbsaw Nbsaw referenced this pull request in Nbsaw/notes Feb 12, 2018

Open

ref={callback} or ref ={string} ? #43

@TrySound

This comment has been minimized.

Show comment
Hide comment
@TrySound

TrySound Feb 12, 2018

Contributor

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

Contributor

TrySound commented Feb 12, 2018

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

@VladislavBaranov

This comment has been minimized.

Show comment
Hide comment
@VladislavBaranov

VladislavBaranov Feb 12, 2018

  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>
    );
  }
};```
  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

This comment has been minimized.

Show comment
Hide comment
@TrySound

TrySound Feb 12, 2018

Contributor

@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.

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.

@arogachev arogachev referenced this pull request in igorprado/react-notification-system Feb 28, 2018

Open

String refs are considered legacy #141

@Dreyer Dreyer referenced this pull request in gcanti/tcomb-form-native Mar 19, 2018

Open

string `refs` are considered legacy #208

@ablamunits ablamunits referenced this pull request in mukeshsoni/react-telephone-input Apr 3, 2018

Closed

Replace string ref usage with callback refs #170

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