MessageTranslator refactor issues

[kahkeng]

I was looking at the new MessageTranslator_, and saw this part below. I guess this is trying to address #105, but this approach I think won't work for the recursive function call pieces, which are used in various places like containers/streaming containers.

Note that we're also creating/re-creating this Map for each call to Widget, and we are instantiating all components/widgets no matter what, even if only one ends up getting rendered.

const Widget = ({ widget }: { widget: Widget }) => {
  const { fnName: fn, args } = widget;
  const fnName = fn.toLowerCase().replace('display-', '');
  const parsedArgs = parseArgsStripQuotes(args);
  const inputString = `${fnName}(${args})`;

  const widgets = new Map<string, JSX.Element>();

  widgets.set(
    'uniswap',
    <Uniswap
      tokenInSymbol={parsedArgs[0]}
      tokenOutSymbol={parsedArgs[1]}
      inputAmount={parsedArgs[3]}
    />
  );

  widgets.set(
    'transfer',
    <Transfer tokenSymbol={parsedArgs[0]} amtString={parsedArgs[1]} receiver={parsedArgs[2]} />
  );

  widgets.set(
    'yield-protocol-lend',
    <YieldProtocolLend
      tokenInSymbol={parsedArgs[0]}
      inputAmount={parsedArgs[1]}
      action="lend"
      projectName="yield-protocol"
    />
  );

  /* If available, return the widget in the widgets map */
  if (widgets.has(fnName)) {
    return widgets.get(fnName)!;
...

I think we should instead be creating this map once, and instead of the component itself, it should be a function call that will accept widget, and the Widget function itself (to allow for recursive function calls).

Something like

// one-time initialization somewhere
const widgets = new Map<string, any>(); // whatever this type should be
widgets.set('uniswap', buildUniswapComponent);

// modular definitions, could be defined in separate files
const buildUniswapComponent = (widget: Widget, Widgetize: any) => {
   ...
}

// module that has a recursive call
const nftAssetTraitsContainer = (widget: Widget, Widgetize: any) => {
  const { fnName: fn, args } = widget;
  const fnName = fn.toLowerCase().replace('display-', '');
  const inputString = `${fnName}(${args})`;

  const { asset, values } = JSON.parse(args);
    return (
          <NftAssetTraitsContainer
            asset={Widgetize({ fnName: asset.name, args: JSON.stringify(asset.params) })}
          >
            {values?.map(({ name, params }: { name: string; params: string }, i: number) => (
              <Fragment key={`i${i}`}>
                {Widgetize({ fnName: name, args: JSON.stringify(params) })}
              </Fragment>
            )) || ''}
          </NftAssetTraitsContainer>
        );
}

// main lookup
export const Widgetize = (widget: Widget) => {
  const { fnName: fn, args } = widget;
  const fnName = fn.toLowerCase().replace('display-', '');
  const inputString = `${fnName}(${args})`;

  if (widgets.has(fnName)) {
    return widgets.get(fnName)!(widget, Widgetize);
  }
  // throw an error for unrecognized widget
  ...
}

[kahkeng]

@sgzsh269 thoughts about this as well?

[sgzsh269]

Re: map of components, I agree that the map should be created once as that would be a more efficient and cleaner approach.

Re: recursive rendering using widgetize approach, it does simplify the logic and has worked well but my one concern here is that it may be tightly coupling the widgets as layout/styling could differ when a widget is stand-alone vs nested so keeping it de-coupled would allow for greater flexibility in rendering the components.

[brucedonovan]

@kahkeng I agree with you on your points. A couple of things to note :

1. component map: I am very much in favour of calling the widget as a function when required. (@marcomariscal correct me if i'm wrong, but I believe there is some tradeoff here though with regards to how react sees/handles the component called directly from a function).
2. RE: recursive functions/containers, the latest UI PR (which ports all the containers/widgets) addresses this point - i hope to have it all ready today.

[kahkeng]

@sgzsh269 I think the layout issues could be addressed in the recursive style if there is a parameter or options dict that is passed into the recursive call, e.g. {addPadding: true}. The inner call could omit that option. I think this might give the most flexibility, if padding etc is needed to be included inside the call to Widgetize (might be necessary since these are presentational components after all). What are you referring to regarding de-coupled? Does that mean not supporting recursive containers?

@brucedonovan I didn't look closely, but I think I saw that there is different handling for ListContainer and StreamingListContainer etc, so only those kinds of containers can hold other containers (i.e. only one level deep vs arbitrary configuration). Previously, we had something like NFTCollectionAssetsContainer which is a container that contains an NFTCollection and a ListContainer of NFTAssets, each of these are also containers. So how would you plan to support these kinds of configs, if we don't have a recursive approach?

I think one major benefit of supporting recursive containers natively (first class support) is that we decouple the frontend implementation from backend, so the backend can (more or less) decide how to structure the data it wants to present, and the frontend will just render it. There is no need to push a frontend change to support new components, and backend can mix-and-match as needed if there are lots of kinds of information that it might want to eventually display (e.g. a larger display consisting of lists, carousels, panels, etc), as long as frontend has first-class support for common paradigms of containers.