String refs cause incorrect warning in ReactTestRenderer

[aweary]

If you attach a ref to an element using a string (ref='foo') and use ReactTestRenderer you get the "Stateless function components cannot be given refs" warning

See #7371 (comment)

This is because getPublicInstance returns null and attachRef requires that the instance be non-null in the warning invariant.

I think we can either return a simple instance instead of null or use some additional check in attachRef to see if we're dealing with ReactTestRenderer. The first option seems reasonable, given the comment in the current code:

ReactTestComponent.prototype.getPublicInstance = function() {
  // I can't say this makes a ton of sense but it seems better than throwing.
  // Maybe we'll revise later if someone has a good use case.
  return null;
};

cc @spicyj

[gaearon]

Proposal:

1. By default return {} there

2. Add a way to inject a custom mock, i.e.

   ReactTestRenderer.create(el, {
    getMockHostInstance(el) {
       // kinda
       return { scrollIntoView() {}, appendChild() {} }
    }
   )

3. Offer a getMockDOMInstance implementation that returns things that look like DOM nodes but don’t do anything

This would make test renderer way more usable. A good test is to verify that it can handle components defined in react-bootstrap.

[aweary]

Offer a getMockDOMInstance implementation that returns things that look like DOM nodes but don’t do anything

What kind of object would getMockDOMInstance return, something that looks like document? If so, users could just return an instance of jsdom making that really easy, especially in Jest.

edit: that might be too ambitious, actually. It does seem better to just let them return an object that implements what ever API they're actually using. So if they're calling node.focus() they can just return { focus: () => {} }

How would that work if they had multiple refs?

[gaearon]

I think an object with no-op methods for complete DOM API might be enough. I don’t think it’s worth making it act more realistic like jsdom. There will always be corner cases for sure but it’s hard to tell what they’re like until we actually ship some version of this and are able to iterate.

[aweary]

I agree. With this API users could return something as sparse or as full featured as they want. As for supporting multiple refs, how would getMockDOMInstance know which instance type it should return?

[gaearon]

@aweary I think it should get relevant element as the argument. Then it can look at element.type or element.props if it wants to.

[aweary]

To clarify, with getMockDOMInstance are you talking about an internally API that provides an object with no-op methods with the complete DOM API, or allowing the user to provide a getMockDOMInstance method like getMockHostInstance so they could return whatever object they want?

[sophiebits]

The main thing I am worried about with returning real DOM nodes is that people will expect children to be there and the full hierarchy to be rendered accurately, and that won't be the case.

[gaearon]

To clarify, with getMockDOMInstance are you talking about an internally API that provides an object with no-op methods with the complete DOM API, or allowing the user to provide a getMockDOMInstance method like getMockHostInstance so they could return whatever object they want?

I mean both. Let user specify their own getMockHostInstance (or maybe getMockRef) as an option.

Separately, provide an implementation of it that returns something with methods like on DOM node (just so it doesn’t throw in most cases). This can be a third party package at first, but if it’s good enough and most people use it, we might as well ship it in react-test-renderer.

It wouldn’t make sense for React Native though which is why I don’t want to just make it the default. Keeping it separate also lets user add special cases:

ReactTestRenderer.create(el, {
 getMockRef(el) {
    if (el.type === 'input' && el.props['data-my-special-snowflake'])
      return getMyWeirdInstance(el);

    return getDOMMockRef(el); // could even look at `el.type` to determine methods to include
 }
)

[aweary]

So my question is, if they didn't provide any getMockRef method, would it just return null like it does now? Meaning it's always the user's responsibility to implement this and optionally use some getDOMMockRef package for default mock objects.

[gaearon]

I think default getMockRef should be return {}.

This is slightly safer (no one ever expects refs to be nulls).
Not that it matters today, but maybe we can make it a Proxy in the future.

It would solve the issue with null warning too.

[sophiebits]

Can we just fix the warning somehow first? My initial impression is that {} is not better than null. You'll likely go a little longer before noticing that the methods are missing and that's likely to be more confusing.

[gaearon]

^^ good point, agree.