mui / toolpad

Toolpad: Full stack components and low-code builder for dashboards and internal apps.
https://mui.com/toolpad/
MIT License
964 stars 243 forks source link

NodeRuntimeWrapper is not required when deploying app? #1035

Closed WangLarry closed 1 year ago

WangLarry commented 2 years ago

Duplicates

Latest version

Current behavior 😯

NodeRuntimeWrapper wrap children with ErrorBoundary and NodeRuntimeContext. It maybe bring a little performance loss for one node. But it is common that there are over 30 nodes in one page of real app.

function RenderedNodeContent({ node, childNodeGroups, Component }: RenderedNodeContentProps) {
  ...

  return (
    <NodeRuntimeWrapper nodeId={nodeId} componentConfig={Component[TOOLPAD_COMPONENT]}>
      {isLayoutNode ? (
        <Component {...props} />
      ) : (
        <Box
          sx={{
            display: 'flex',
            alignItems: boundLayoutProps.verticalAlign,
            justifyContent: boundLayoutProps.horizontalAlign,
          }}
        >
          <Component {...props} />
        </Box>
      )}
    </NodeRuntimeWrapper>
  );
}

Expected behavior πŸ€”

No response

Steps to reproduce πŸ•Ή

Steps:

1. 2. 3. 4.

Context πŸ”¦

No response

Your environment 🌎

No response

apedroferreira commented 2 years ago

Thanks for reporting this issue @WangLarry. Not sure how plausible it would be to not to wrap each component with this - we'd also have to measure how much this is affecting performance to see if it's worth tackling.

Janpot commented 1 year ago

I believe it's needed at runtime because we want to be able to show error information on a per-component basis, even in the application runtime. This to avoid crashing the whole application and losing context of where an error is happening.

WangLarry commented 1 year ago

NodeRuntimeContext inside of it is unnecessary.

Janpot commented 1 year ago

NodeRuntimeContext inside of it is unnecessary.

At the moment, it's indeed only used by the editor. But unless there's a significant measurable performance hit, I think we can live with it, the context value should be stable for the lifetime of the component, so I don't expect there to be any problem. I believe there are more important performance issues to focus on in the application runtime.