base-ui icon indicating copy to clipboard operation
base-ui copied to clipboard

[core] Improve code readability when using mergeProps

Open mnajdova opened this issue 8 months ago β€’ 1 comments

A proposal of how we can write more readable code when it comes to using mergeProps. There is one subtle issue with the typings, if I use React.ComponentsPropsWithRef<'div'> on the defaultProps const, it fails because of our refs, as they are always defined as React.useRef<HTMLElement | null>, so I just kept the type as is, but added the event handlers event types. Let me know if you think we should change this.

In addition I don't think we need to always define the new const, maybe just in places where the code is not readable. I changed two examples places, if we agree on the pattern I will do the changes on the rest codebase.

mnajdova avatar Mar 06 '25 09:03 mnajdova

Deploy Preview for base-ui ready!

Name Link
Latest commit 8bbe9648d8c11395fbd86b7a6f0da1f7491984bd
Latest deploy log https://app.netlify.com/sites/base-ui/deploys/67c98854bd3d9c0008b5fc0f
Deploy Preview https://deploy-preview-1540--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 site configuration.

netlify[bot] avatar Mar 06 '25 09:03 netlify[bot]

IMO what we have currently is OK and I don't see a problem with readability. The proposed solution introduces an additional useMemo call, that can have a slight perf impact.

michaldudak avatar Jul 02 '25 11:07 michaldudak

Agree with MichaΕ‚. However there's one thing that this change highlights that I like: if it can be a value (rather than a function), it should be a value. Imho, mergeProps is used too much and some of those components should be returning their props directly rather than a prop getter. The issue is that calling mergeProps multiple times (each time a new set of props need merging) is slower than calling it once (when we have all the props sets to be merged together).

romgrk avatar Jul 03 '25 18:07 romgrk

Likely the only time we need to return a function is when anything below uses useButton as merging its props requires access to other external props (i.e. the onKeyDown prop needs to access external onClick). Unfortunately, useButton is pretty widely used .

michaldudak avatar Jul 04 '25 11:07 michaldudak