Warn for functional refs on stateless functional components

[aweary]

Resolves #7267

Currently React warns if you do:

<StatelessComponent ref='foo' />

But it doesn't warn if you do

<StatelessComponent ref={node => { /* ... */ }} />

It seems like a generally good idea to have a consistent warning when using refs with any SFC.

[pke]

Maybe keep it consistent with the docs and name it "Stateless functional components"?

[aweary]

This warning is consistent with the current invariant in ReactCompositeComponent: https://github.com/facebook/react/blob/master/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js#L1182

[aweary]

But I do think I should probably include the component/ref name just like https://github.com/facebook/react/blob/master/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js#L1187-L1190

[pke]

Yes, knowing which component has the invalid ref would help a lot.

[satya164]

What if I pass a function from parent and use it for the ref? String refs won't work. But I think function refs do.

[aweary]

@satya164 do you mean like passing a function down to a child via the ref property so it can be used to attach a ref to a child component that isn't stateless?

If it's a stateless component it will always pass null to whatever function is provided in the ref prop, so it wouldn't actually provide any benefit, even if the passed function is bound to a stateful parent.

[satya164]

@aweary Ah.

[edvinerikson]

IMO this should follow the normal pattern check the render method of ...
Something like Stateless functional component %s cannot have refs attached. Check the render method of %s. I also think that the other message (when using a string ref) should follow this pattern too.

[edvinerikson]

Someone from FB should probably review this too.
It looks pretty good but I think that we should have a invariant if we try to create a ref inside a stateless component too (this PR only warns if we try to put a ref on a stateless component).

function A() {
  return <B ref="B-ref" /> // invariant called
  return <B ref={() => {}} /> // invariant is not called
}

[aweary]

It looks pretty good but I think that we should have a invariant if we try to create a ref inside a stateless component too (this PR only warns if we try to put a ref on a stateless component).

@edvinerikson React should already warn for

return <B ref="B-ref" />

This PR actually implements what I think you're describing, if I'm understanding you correctly. If you look at the test, you can see what this PR is addressing:

return <StatelessComponent name="A" ref={() => {}}/>;

Currently, React will call the ref function with whatever is returned from getPublicInstance which is always null for these SFCs, this PR checks to see if that instance is null or not and throws if it is.

[edvinerikson]

What I tried to explain is that if you use ref inside a stateless functional component it will not warn that you do that. However it warns that you have attached a ref to a stateless component.

class AClass extends Component {}

function FunctaionalComponent() {
  return <AClass ref={() => {}} />
// this won't warn right now but it should because using refs inside a stateless
// component shouldn't be possible, right? (Not possible with string refs at least)
}

Edit:
Using a function as ref is probably a valid use case

[aweary]

@edvinerikson thanks for clarifying, that should be doable by checking the instance for owner as well.

[aweary]

It's currently failing on a couple tests using ReactTestUtils, such as ReactTestUtils-test.js#L215-L232 as the component being passed to attachRef is returning null for getPublicInstance. So I'll have to address that

It seems like ReactTestUtils eventually just returns an instance of NoopInternalComponent which will always return null for getPublicInstance, so this warning is going incorrectly fire when using the shallow renderer.

[edvinerikson]

It's currently failing on a couple tests using ReactTestUtils, such as ReactTestUtils-test.js#L215-L232 as the component being passed to attachRef is returning null for getPublicInstance. So I'll have to address that

I guess that maybe you can check if component._currentElement.type is a function, might be hacky though

[aweary]

I guess that maybe you can check if component._currentElement.type is a function, might be hacky though

Yeah, I'd hate to do that just because of ReactTestUtils. I wonder if we can consider actually returning an instance for ReactShallowRenderer, at least in some cases. Maybe createRenderer could accept a configuration object that lets users enable this. I'm not sure if that would break anything though.

[edvinerikson]

Someone from FB will probably need to decide what to do.
cc @zpao

[gaearon]

@edvinerikson

// this won't warn right now but it should because using refs inside a stateless
// component shouldn't be possible, right?

It is possible for callback refs.

class AClass extends Component {}

function FunctaionalComponent() {
  var inst;
  return <AClass ref={a => inst = a} />
}

This is why callback refs are better than string refs.

[gaearon]

@aweary Would you like to revive this?

[aweary]

@gaearon rebased. There are a few failing tests for ReactTestRenderer and ReactTestUtils that I'll look at soon 👍

[gaearon]

Ping :-)

[gaearon]

Actually I think this is superseded by #8635 (which also adds them for Fiber).
Thanks for your work though!