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

[core] refactor: useModernLayoutEffect => useLayoutEffect

Open romgrk opened this issue 6 months ago • 9 comments

Following this discussion: https://mui-org.slack.com/archives/C011VC970AW/p1738093359898899

The "modern" token doesn't bring any useful semantic, and makes the codebase more verbose. I've also added jsdocs & typings in our re-export because somehow the typings and jump-to-definition don't work for the floating-ui package, so I had to manually find that function to find what it does.

romgrk avatar May 15 '25 23:05 romgrk

In my opinion, the Modern token is useful to disambiguate it from the native React.useLayoutEffect (to avoid accidentally using that), especially when auto-importing it. Floating UI auto imports work fine for me fwiw

atomiks avatar May 15 '25 23:05 atomiks

Open in StackBlitz

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

commit: 876d7fe

pkg-pr-new[bot] avatar May 15 '25 23:05 pkg-pr-new[bot]

Deploy Preview for base-ui ready!

Name Link
Latest commit 876d7fe3236022b2938e26921b8ff6f0d9bbe8e9
Latest deploy log https://app.netlify.com/projects/base-ui/deploys/682673e00e60d60008dc7675
Deploy Preview https://deploy-preview-1931--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.

netlify[bot] avatar May 15 '25 23:05 netlify[bot]

is useful to disambiguate it from the native React.useLayoutEffect

We strictly enforce using React imports with React.[...] via an eslint rule, so it's always unambiguous that it's not the React version.

to avoid accidentally using that

If we're really worried about that case, we should add an eslint rule to disable using that react import. Otherwise we're still relying on people not forgetting to use the right one instead of automatically reaching for React.useLayoutEffect.

I don't feel like those reasons aren't compelling enough for the increase in verbosity. The more characters there are on screen, the less the eye can focus on what actually matters.

romgrk avatar May 16 '25 00:05 romgrk

I don't feel like those reasons aren't compelling enough for the increase in verbosity. The more characters there are on screen, the less the eye can focus on what actually matters.

Usually but it's also slightly less ergonomic to import since react has priority

Screenshot 2025-05-16 at 11 46 58 am

atomiks avatar May 16 '25 01:05 atomiks

Code is read considerably more times than it is written, it feels preferable imo to optimize for readability rather than save 2 clicks when importing that hook.

romgrk avatar May 16 '25 06:05 romgrk

We could also consider the shorter useIsoLayoutEffect, which makes it non-ambiguous where the function is coming from (the use-isomorphic-layout-effect package), though I still think useLayoutEffect is better. The "iso" or "isomorphic" isn't really accurate, at least since https://github.com/Andarist/use-isomorphic-layout-effect/pull/7 where the server-side variant is a noop.

romgrk avatar May 29 '25 23:05 romgrk

I don't have strong opinion on either. useIsoLayoutEffect seems like a good compromise.

michaldudak avatar Jun 20 '25 14:06 michaldudak

I strongly prefer useLayoutEffect, because I don't think the semantic brings anything useful. The difference is just about the warning message (AFAIK), and I don't think avoiding a warning message justifies modifying the semantic of "useLayoutEffect" throughout the codebase.

@atomiks Any other reason to prevent the change? The ambiguity is not applicable, and the import-DX feels less important than the reading-DX, so I don't see a strong reason to avoid the change. I can set up an eslint rule to prevent using useLayoutEffect from react.

I'll probably redo this PR on top of the separate utils package PR once it's merged.

romgrk avatar Jun 27 '25 18:06 romgrk

I don't feel that I read code more than I write it in this repo, at least currently. We're building a lot of new components. I feel like I'm always typing useMod- a lot and want to have it auto-imported without problem; it's one less point of friction to me

atomiks avatar Jun 28 '25 02:06 atomiks