Skip to content

[core] refactor: useModernLayoutEffect => useLayoutEffect #1931

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 3 commits into
base: master
Choose a base branch
from

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented May 15, 2025

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 romgrk added core Infrastructure work going on behind the scenes improvement This is not a bug, nor a new feature labels May 15, 2025
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 15, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 15, 2025
@atomiks
Copy link
Contributor

atomiks commented May 15, 2025

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

Copy link

pkg-pr-new bot commented May 15, 2025

Open in StackBlitz

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

commit: 876d7fe

Copy link

netlify bot commented May 15, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit d0cab6e
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/682673a04712fd0008e72673
😎 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.

Copy link

netlify bot commented May 15, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 876d7fe
🔍 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.

@romgrk
Copy link
Contributor Author

romgrk commented May 16, 2025

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.

@atomiks
Copy link
Contributor

atomiks commented May 16, 2025

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 16, 2025
@romgrk
Copy link
Contributor Author

romgrk commented May 16, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes improvement This is not a bug, nor a new feature PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants