[useRender] Add defaultTagName parameter#2527
Conversation
commit: |
Bundle size report
|
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| renderParams.disableStyleHooks = true; | ||
|
|
||
| return useRenderElement(undefined, renderParams, renderParams); | ||
| return useRenderElement(renderParams.defaultTag, renderParams, renderParams); |
There was a problem hiding this comment.
The terminology feels wrong. "tag" does not seem part of the React language.
If we follow https://legacy.reactjs.org/docs/implementation-notes.html#mounting-host-elements:
| return useRenderElement(renderParams.defaultTag, renderParams, renderParams); | |
| return useRenderElement(renderParams.defaultHostElement, renderParams, renderParams); |
| return useRenderElement(renderParams.defaultTag, renderParams, renderParams); | |
| return useRenderElement(renderParams.defaultIntrinsicElement, renderParams, renderParams); |
There was a problem hiding this comment.
But I wonder, why only support host elements:
| return useRenderElement(renderParams.defaultTag, renderParams, renderParams); | |
| return useRenderElement(renderParams.defaultComponent, renderParams, renderParams); |
There was a problem hiding this comment.
Yeah I don't know the best word for this 😬
I find the word "host" to be pretty abstract, and sort of implies knowledge of how React's internals work, while HTML "tag" is common knowledge. I also see the word "element" representing the object value when writing JSX, not a string. tag matches ref.current.tagName, except in lowercase
So, maybe defaultTagName.
But I wonder, why only support host elements
It's a shortcut for rendering the JSX element as a function and doesn't need to support components. The render prop itself is for that purpose
There was a problem hiding this comment.
"tag" sounds fine to me, if that prop only supports HTML tag names.
and doesn't need to support components. The render prop itself is for that purpose
render: <Something /> is more expensive, it forces more memory allocations and a React.cloneElement() call. Being able to pass Something the same way we can pass "div" would also allow better perf in the cases where it matters. And it could solve the naming topic by forcing this param to be named defaultComponent.
There was a problem hiding this comment.
The useRender hook is specifically for building primitive components where you aren't rendering another component (yet). The callsite of the component that has useRender is where they do the replacing with render
There was a problem hiding this comment.
The useRender hook is specifically for building primitive components where you aren't rendering another component (yet)
I wonder if we won't see cases where we need to expand the need to support any components. From what I understand of https://github.com/radix-ui/primitives/blob/36d954d3c1b41c96b1d2e875b93fc9362c8c09e6/packages/react/slot/src/slot.test.tsx, this is supported.
not a string. tag matches ref.current.tagName, except in lowercase
Right, ok, there is a way to justify the name, I framed this too strongly with "wrong" 😁.
There was a problem hiding this comment.
I wonder if we won't see cases where we need to expand the need to support any components.
And if we pick defaultComponent for this param name now, we could later expand the typings without a breaking change.
There was a problem hiding this comment.
I would use defaultTag as it's the only things we support now. If in the future we want to extend the behavior, we can add a new option defaultComponent and deprecate the defaultTag in favor of it. It would feel wrong naming it defaultComponent if it does not support custom components.
There was a problem hiding this comment.
It would feel wrong naming it defaultComponent if it does not support custom components.
Agree
There was a problem hiding this comment.
If in the future we want to extend the behavior
@mnajdova Right, I was mostly wondering about either there is a use case or not for any components.
I have tried to push the reflection on it, but I can't find a solid argument for extending support. The rationale I land on: this utile is slow, so we only want to allow React tree leaves to be swapped with it. For an element in the middle of the tree, instead, we can use a slot or composition.
atomiks
changed the title
defaultTag parameterdefaultTagName parameter
Signed-off-by: atomiks <cc.glows@gmail.com>
6 participants
Adds a
defaultTagName: typeof React.JSX.IntrinsicElementsproperty to theuseRenderoptions, instead of needing to set a default for therenderoption (usually a React element), which causes the hook to useReact.cloneElementand is a slower default for perf sensitive primitivesPreview: https://deploy-preview-2527--base-ui.netlify.app/react/utils/use-render.