-
-
Notifications
You must be signed in to change notification settings - Fork 322
[infra] Extract utilities to a separate package #2167
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
Conversation
Bundle size report
|
That means they will be semver stable? Should we document them? cc @romgrk to get your attention on this PR, I believe you brought up the topic of shared libraries recently. |
packages/react-utils/README.md
Outdated
| @@ -0,0 +1,3 @@ | |||
| # @base-ui-components/react-utils | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd go with just @base-ui-components/utils for the name. The package contains more than just react-specific utils, and the name is already long due to the prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm fine with it too. I'm wondering if it shouldn't be internal-utils, though, to match the convention we use in other such libraries (@Janpot, any opinions?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the current conventions, that would make it semver unstable and then it can only be depended in this repo, and only if the dependency is defined with exact version.
I think @base-ui-components/utils makes sense, just don't make breaking changes in it on a minor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That means they will be semver stable? Should we document them?
Yes it should follow semver, and no I don't think we should document them beyond jsdoc.
commit: |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| useSpringRef, | ||
| } from '@react-spring/web'; | ||
| // eslint-disable-next-line no-restricted-imports | ||
| import { useTransitionStatus } from '@base-ui-components/react/utils/useTransitionStatus'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be -
| import { useTransitionStatus } from '@base-ui-components/react/utils/useTransitionStatus'; | |
| import { useTransitionStatus } from '@base-ui-components/utils/useTransitionStatus'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, useTransitionStatus is Base UI-specific and lives in the main package.
| import * as ReactDOM from 'react-dom'; | ||
| import { useScrollLock } from '@base-ui-components/react/utils'; | ||
| // eslint-disable-next-line no-restricted-imports | ||
| import { useScrollLock } from '@base-ui-components/react/utils/useScrollLock'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
LukasTy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Thank you for exporting these. 🙏
Leaving some minor nitpicks.
I've tested it on mui/mui-x#18734 and have a question.
Have you considered exporting useButton hook? 🤔
| useSpring, | ||
| useSpringRef, | ||
| } from '@react-spring/web'; | ||
| // eslint-disable-next-line no-restricted-imports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this package does not have problems importing files from more than 2 levels deep, then maybe the ESLint rules should be adjusted? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't really import these from the package. Docs use Base UI sources directly, so they can access private exports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs use Base UI sources directly
Is this avoidable? this tends to mess up caching algorithms that rely only on npm dependencies. and all kinds of other subtle problems, e.g. typescript filles outside of the root folder and stuff like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does create subtle problems, but it also makes the devmode DX much nicer. HMR works seamlessly if you use source files rather than built files, and you can import anything you want. The setup is harder to maintain, but the tradeoff is worth it, imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HMR works seamlessly if you use source files rather than built files
I think for this case, we should be using a watch script for typescript builds of the lib. HMR should work fine on the built files after they are updated by the typescript watcher.
I think another good purpose of the docs is testing that our packaging is working properly. I could see an issue if there were subtle differences in the typescript config between the docs and the lib.
Also, why do the docs need to access private exports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if necessary we can alias to sources for dev mode, I don't mind so much, as long as the aliasing can be removed and no code changes are necessary for regular pnpm workspaces resolution to work correctly.
each package building in watch mode has my preference, but we need to make sure it can scale to the size of core and X monorepos. We'll likely start work on a vite configuration for building our libraries soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could see an issue if there were subtle differences in the typescript config between the docs and the lib.
I have witnessed such issues in the past, but I argued unsuccessfully for running our test suites (rather than the docs) in prod mode to deal with that.
Also, why do the docs need to access private exports?
Private experiments that we don't want to publish, such as this case and anything else under .../(private)/experiments/.
I think for this case, we should be using a watch script for typescript builds of the lib. HMR should work fine on the built files after they are updated by the typescript watcher.
If it can be shown to not affect the DX, I'm fine with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this avoidable? this tends to mess up caching algorithms that rely only on npm dependencies. and all kinds of other subtle problems, e.g. typescript filles outside of the root folder and stuff like that
We can discuss it on the infra meeting. It's not in scope of this PR anyway.
packages/react/src/floating-ui-react/components/FloatingPortal.test.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com> Signed-off-by: Michał Dudak <michal.dudak@gmail.com>
We might actually have a Button component at some point, but it's not fully decided yet. |
Extracted utilities that can be used in other MUI projects to a separate library - @base-ui-components/utils.
Before we start using it across the company, I want to update JSDocs and possibly rename and reorganize few utilities.