Skip to content

[core] Performance/refactor: useRender #1934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented May 16, 2025

This is both perf/refactor. Make useRender match the useRenderElement API.

@romgrk romgrk added core Infrastructure work going on behind the scenes improvement This is not a bug, nor a new feature performance labels May 16, 2025
Copy link

pkg-pr-new bot commented May 16, 2025

Open in StackBlitz

npm i https://pkg.pr.new/@base-ui-components/react@1934

commit: 43070a3

Copy link

netlify bot commented May 16, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 43070a3
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/6827b17b48ffb400086049a0
😎 Deploy Preview https://deploy-preview-1934--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Comment on lines 15 to 22
const { render, props, state, refs } = params;
const { ref: intrinsicRefProp, ...intrinsicProps } = props || {};
const renderParams = params as useRender.Parameters<State, RenderedElementType> & {
disableStyleHooks: boolean;
};
renderParams.disableStyleHooks = true;

const renderElement = useRenderElementLazy(
undefined,
{ render },
{
props: intrinsicProps,
state,
ref: [intrinsicRefProp, ...(refs || [])].filter(
(x): x is React.Ref<RenderedElementType> => x != null,
),
disableStyleHooks: true,
},
);
const element = useRenderElement(undefined, renderParams, renderParams);

return {
renderElement,
};
return element;
Copy link
Contributor Author

@romgrk romgrk May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't insist on the mutable .disableStyleHooks line, I just wrote it this way to highlight what this refactor made evident: useRender is really just useRenderElement but with style hooks turned off.

The prop/ref destructuring & handling logic was all unnecessary: it's duplicated with the logic in useRenderElement which already does all that.

Comment on lines -66 to +57
* Refs to be merged together to access the rendered DOM element.
* The ref to apply to the rendered element.
*/
refs?: React.Ref<RenderedElementType>[];
ref?: React.Ref<RenderedElementType> | React.Ref<RenderedElementType>[];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I standardized this with useRenderElement, both use ref for the propname, while still allowing an array.

Copy link
Contributor

@atomiks atomiks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the conditional rendering optimization with an early null return, or was that not much of a concern in your perf tests?

@romgrk
Copy link
Contributor Author

romgrk commented May 16, 2025

What about the conditional rendering optimization with an early null return, or was that not much of a concern in your perf tests?

I have not seen a case where the lazyness was actually needed, we seem to call renderElement() right away every time. If we do need it, we can introduce a matching wrapper for useRenderElementLazy.

@atomiks
Copy link
Contributor

atomiks commented May 16, 2025

Searching if (!shouldRender) reveals most of the cases where an early return is used.

@romgrk
Copy link
Contributor Author

romgrk commented May 16, 2025

Ah right, I didn't look at the deprecated useComponentRender. I've added the lazy variant here and updated the docs.

There could be more lazyness though. Laziness is set just at the evaluateRenderProp() boundary, but the whole props merging logic before that could be avoided as well. I'll keep this PR focused on the current change though, I'll do that change separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change core Infrastructure work going on behind the scenes improvement This is not a bug, nor a new feature performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants