[all components] Support lazy element in render prop#3856
[all components] Support lazy element in render prop#3856atomiks merged 8 commits intomui:masterfrom
render prop#3856Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
0e36d98 to
f41510f
Compare
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
47b1626 to
38d0fa0
Compare
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
38d0fa0 to
3d6972e
Compare
3d6972e to
49dccc7
Compare
atomiks
left a comment
There was a problem hiding this comment.
Nice that there's a workaround 🙌 Have some notes:
| `Base UI: Expected to receive a valid React element in the "render" prop but received an invalid one.`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could this error message be more helpful? e.g. why is it invalid, how to fix it.
There was a problem hiding this comment.
Ok, I tried to do something better, applying:
There was a problem hiding this comment.
Suggestion, add error writing style in your AGENTS.md like mui/material-ui#47668 so that review bots can suggest improvements.
82bcea8 to
a227d2b
Compare
a227d2b to
c5457e7
Compare
Signed-off-by: atomiks <cc.glows@gmail.com>
| // so we can throw before with custom message. | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| if (!React.isValidElement(newElement)) { | ||
| throw /* minify-error-disabled */ new Error( |
There was a problem hiding this comment.
Why not just remove that [].join('\n')?
There was a problem hiding this comment.
We don't need to minify the error, it won't run in production.
There was a problem hiding this comment.
Right, but why not write error messages according to the same rules, regardless of whether there's a production guard around? And why does it matter that this error does not get minified?
Anyway, we can probably automate this check like in mui/mui-public#794
There was a problem hiding this comment.
but why not write error messages according to the same rules
Fair point
why does it matter that this error does not get minified?
We still need /* minify-error-disabled */ to ignore that error.
There was a problem hiding this comment.
Taking care of non-prod errors in mui/mui-public#1069
render prop
|
Possibly related to this: support for a <Component render={<Lazy />} />
<Component render={(props) => <Lazy {...props} />} /> |
@atomiks AFAIK, this already work; it doesn't go through the |
Fix #2859. See #2859 (comment) for why a fix. With this PR release, we could then merge shadcn-ui/ui#9444.
You can test this work / the problem in the playground using:
While I was at it, I tried to bring it to the next step. There were easy, low-hanging fruits 🍇:
Tests
I have added some missing test cases for
useRenderElement().Error recovery
I have added logic to help developers when they provide wrong values to the
renderprop.Before, if you did this:
<Button nativeButton={false} render={<React.Fragment>Bar</React.Fragment>}>, the error would be confusing:now, you get something clearer. I love it 😌: