Add the ability for consumers to render custom components

[james-woo]

Allow consumers of RemoteRenderer to pass in their own custom component renderer or text renderer.

This will allow React Native to override the default behaviour of text rendering, since React Native requires that all Text objects to be wrapped in a ReactElement.

For web, we can remove the hack that gets the __type__ out of the component and move it back to web's side by overriding the component rendering.

[lemonmade]

Do we have to clone the element, or could the renderText/ renderComponent call be expected to do that? This component is rendered for every component in the tree so keeping it as lightweight as possible is definitely desirable

[henrytao-me]

The main issue is we are looping through the children which requires key. Is there a way to get around it?

[lemonmade]

Can't renderComponent add the key?

[lemonmade]

Does this need to be a separate concept from the Controller, which already has a very related job of mapping remote component names => host component implementations?

[henrytao-me]

Controller is meant for rendering component type. We also has Text Component as component type which will be rendered using text type. This renderText is for text type.

Are you saying we should reserve a special Component in components list to render text type? This is what @vividviolet suggested before.

[lemonmade]

Controller can be whatever we like it to be. Right now, it just gives the component implementation for a remote component, but I think it makes sense for createController to accept a second argument with the renderText stuff you've added here, and have those properties be exposed publicly such that we don't need a second piece of context.

[henrytao-me]

We could do it 👍

[lemonmade]

Same question regarding the need to clone the element

[henrytao-me]

It's about the key

[lemonmade]

ReactElement is a type, so should be a type-only import

[lemonmade]

We usually use method syntax instead of property syntax for these

[henrytao-me]

Looks great 👍

[lemonmade]

This looks really good, just a few niggles/ suggestions left from me

[henrytao-me]

Specifically this ⬆️

This is how the consumer can use the api

createController(components, {
  componentRenderer: (defaultRenderer) => (props) => {
    const {component} = props;
    switch(component.type) {
       case 'Card': 
         // Don't render Card
         return null;
       case 'Stack':
         // return custom Stack component
         break;
       default:
          return defaultRenderer(props);
    }
  }
});

[lemonmade]

Looking at this with fresh eyes, this API feels a little too complex and verbose. I would prefer this:

createController(components, {
  renderComponent(component, {key, renderDefault}) {
    switch (component.type) {
      case 'Card': return null;
      case 'Stack': return <CustomStackComponent {...component.props} key={key} />;
      default: return renderDefault();
    }
  }
});

We get rid of the "arrow function returning arrow function" business and move some of the arguments around so that component/ text can be the first positional argument, which lets consumers destructure a little more confidently since they never need to pass the arguments they get back into another function. What do you think?

[henrytao-me]

Do you think we need to make the usage of key transparent? The consumer of this renderComponent should not know about why they need the key. If they forget to use key, it will complain because the part that needs it is in RemoteRenderer. Other than that, the approach looks good to me

[lemonmade]

The only way I can think to avoid needing to worry the consumer about the key would be to wrap each spot where we call these renderers in a <Fragment key={}></Fragment>. Not sure if that will work for your use case, and it's a bit annoying that we add an extra wrapping fragment around every component we create, but maybe it's OK?

[henrytao-me]

That should be okay. Let's do it @james-woo. Action items:

• Use Fragment for the key
• Change signature to renderComponent(component, {renderDefault}). I think we still keep renderDefault in an object for future extensibility.

[james-woo]

Removed Fragment as this wasn't compatible with passing the __type__ from web layer. Instead the consumer will have to remember to forward the key.

[henrytao-me]

👍

[henrytao-me]

Tested with James. LGTM 👍