[WIP] Set _hostNode in mountComponent / receiveComponent

[aweary]

Adds support for findDOMNode with ReactTestRenderer, initially mentioned here.

[aweary]

@gaearon not sure how to address this, but in receiveComponent the transaction is not pulled from the pool with the test options like it is during the initial mount. This means this fails since options ends up being true instead of the options object we'd want.

Injection makes this kind of a hard case, so I'm wondering what you think we can do here. It seems like we'd have to either attach the test options to the instance and access it there or do more injection, but both of those sound less than ideal.

[gaearon]

Okay. 😞

Can we use this._hostContainerInfo for this instead? It's a thing that DOM renderer uses, for example, for DOM nesting validation. Both composite and host instances already store it so it's always available and passed all the way down. It is the fourth argument to ReactReconciler.mountComponent(), third argument to mountComponent() implementations, and I think it's currently null in the test renderer.

[gaearon]

@spicyj Can we wait with 15.4.0 until this is addressed? I'll try to make sure this gets done by Friday.

[aweary]

@gaearon I can look at this again today and likely get it finished 👍

[sophiebits]

Do you imagine that people use ReactTestRenderer.findDOMNode? If so, let's add that and change the test to use that.

[gaearon]

@spicyj I think it should work with ReactDOM.findDOMNode because third party components may be using it with refs.

[sophiebits]

Hm, my gut reaction is that we should not support that. The renderers should be independent.

[aweary]

If we didn't support that then you wouldn't be able to use the test renderer with components that utilize findDOMNode, which includes a number of popular third party component libraries

[sophiebits]

Do they have to? Can we get them to change? :)

[aweary]

Can we get them to change? :)

In an ideal world 😀. I think we should support it as long as using findDOMNode is supported, but I'll defer to you guys.

[gaearon]

Not likely: jsx-eslint/eslint-plugin-react#678 (includes comments from React Bootstrap maintainers).

[aweary]

So do we want to move forward with this?

[gaearon]

We need to cut 15.4.0 soon so if you don't see a way to fix this, we'll proceed without it.
Unfortunately nobody on the core team has the time to look into this now.

[aweary]

@gaearon I'm pretty sure I can fix it, I just wasn't sure there was agreement on if we should support this or not.

[gaearon]

I'm a bit confused about this solution. Can't user-defined createNodeMock just return an object with nodeType === 1 and then findDOMNode() would pass it through?

[gaearon]

I don't think adding support to test renderer for findDOMNode() is the right solution. The right solution is to fool findDOMNode() into thinking mock node is a DOM node (which is the desired behavior anyway).

[gaearon]

Nevermind, I understand why this wouldn't work for composites now.

[gaearon]

I looked at it again and I'm inclined to agree with @spicyj this is not supported.

For example, we may want to ship ReactDOM with Fiber under the hood, in which case it is by definition incompatible with test renderer. Even if we couple them now with some convention, it's going to break in weird ways eventually, and we wouldn't want to do it when people rely on it in tests.

So unfortunately it seems like snapshot testing can't be used with libraries that rely on findDOMNode(compositeInstance).

[aweary]

@gaearon thanks for expanding, that makes sense to me too. It wouldn't make sense to support it now if it's fundamentally incompatible with Fiber.

At least we tried 😄

[singpolyma-shopify]

So, any component using findDOMNode makes it so my React app cannot be tested?

[gaearon]

Not testable specifically with snapshot testing. You can still use other testing strategies (e.g. shallow rendering, or rendering into jsdom).