Remove canonical check in fiber host component

[elicwhite]

I believe this is a dead branch.

I added this code in #15126 and I think it has always been wrong. At least, I'm not sure what could trigger it.

I think I was pattern matching from this code in NativeMethodsMixin https://github.com/facebook/react/blob/master/packages/react-native-renderer/src/NativeMethodsMixin.js#L166-L182

Simplified:

const maybeInstance = findHostInstance(this);
if (maybeInstance == null) {
  return;
}

if (maybeInstance.canonical) {

The only thing in the codebase that creates an object with a canonical key is createInstance in ReactFabricHostConfig. canonical is only set on Fabric host components, not paper ones.

In order to get that instance, you must call findHostInstance. NativeMethodsMixin and ReactNativeComponent both use findHostInstance in order to detect whether an instance is created with the paper renderer or the fabric renderer to know which API to call.

FiberHostComponent only needs to deal with paper components and the FabricHostComponent only needs to deal with the fabric components, so this check for canonical in paper is unnecessary.

When I wrote this code I also added tests for calling this function passing an argument created with all three NativeMethodsMixin, ReactNativeComponent, and a host component. If I add a throw to this if statement, none of the tests fail.

[gaearon]

What if someone passes a Fabric instance to Paper's measureLayout ?

[elicwhite]

Is that possible?

I know there is some cross calling that can happen as demonstrated in ReactFabricAndNative-test.internal.js however those cases are different.

All the cases in those tests are from calling the public exports of the renderer with a Fabric instance which happens all the time today, in fact, nothing is calling the public exports of the Fabric renderer in JS.

import {findNodeHandle} from 'react-native'; // This is always from the paper renderer

let ref = React.createRef();
const Component = 'RCTView';
ReactFabric.render(<Component ref={ref} />, 11);

// This method has to work with paper and fabric instances
let handle = ReactNative.findNodeHandle(ref.current);

This has to continue working, but I'm not sure how in practice you'd call a method on a paper instance with a fabric instance. Are you able to have two subtrees (or roots? 😮) one in paper and one in Fabric?

Like, I'm sure a test like this would trigger the problem, but is this a case that could actually happen / we should think about?

import {findNodeHandle} from 'react-native'; // This is always from the paper renderer

let fabricRef = React.createRef();
let paperRef = React.createRef();
const Component = 'RCTView';
ReactFabric.render(<Component ref={fabricRef} />, 11);
ReactNative.render(<Component ref={paperRef} />, 21);

paperRef.measureLayout(fabricRef);

[gaearon]

I haven't thought about it much and it's pretty late. :-) I'd double check with Seb but you have more context on this.

[elicwhite]

Something I realized this morning is that this test wouldn't actually trigger that code:

import {findNodeHandle} from 'react-native'; // This is always from the paper renderer

let fabricRef = React.createRef();
let paperRef = React.createRef();
const Component = 'RCTView';
ReactFabric.render(<Component ref={fabricRef} />, 11);
ReactNative.render(<Component ref={paperRef} />, 21);

paperRef.measureLayout(fabricRef);

because the instance you get back from a fabric ref doesn't have a canonical key.

I added console.log(Object.keys(viewRef)); to one of the Fabric tests and get these results back for the three different types of components in Fabric:

// HostComponent
      [ '_nativeTag',
        'viewConfig',
        'currentProps',
        '_internalInstanceHandle' ]

// ReactNativeComponent
      [ 'props',
        'context',
        'refs',
        'updater',
        '_reactInternalFiber',
        '_reactInternalInstance',
        'state' ]

// NativeMethodsMixin
      [ 'measure',
        'measureInWindow',
        'measureLayout',
        'setNativeProps',
        'focus',
        'blur',
        'props',
        'context',
        'refs',
        'updater',
        'state',
        '_reactInternalFiber',
        '_reactInternalInstance',
        '__isMounted' ]

So passing a fabric ref to the paper function wouldn't enter the canonical branch. The only time you need the canonical check is when explicitly using findHostInstance inside of the react renderer, which this function isn't using.

---

Also, I tried really hard to validate this with an actual test and couldn't write one. I tried adding something to the ReactFabricAndNative-test.js file but get errors about the view config not being registered, this is due to jest.resetModules() being called. If I don't call that the test fails to import both renderers:

 Could not import the "native" renderer in this suite because another renderer has already been loaded earlier. Call jest.resetModules() before importing any of the following entry points:

      * react-native-renderer

It seems like having multiple renderers running like this is not an explicitly supported approach.

---

@gaearon, thanks for pushing me to deeply understand this even more. I feel even more confident in this change now.

[bvaughn]

I don't feel like I have as much context as you do on this, Eli. Stamping to unblock for now. We can always roll this change back if it causes problems after a sync.