-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
chore: use catalog for dependencies across multiple packages and the website #7824
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #7824 +/- ##
==========================================
- Coverage 75.48% 75.46% -0.03%
==========================================
Files 101 101
Lines 8309 8309
Branches 218 218
==========================================
- Hits 6272 6270 -2
- Misses 2035 2037 +2
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
catalog: | ||
'@types/node': 22.15.3 | ||
'@types/react': ^19.1.0 | ||
classnames: ~2.5.1 | ||
react: ^19.1.0 | ||
tailwindcss: ~4.0.17 | ||
typescript: ~5.8.2 |
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.
These were the dependencies I saw repeated more than once across all the packages and the website. Feel free to suggest any that might be missing.
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.
When the packages are published, will these be replaced with normal versions?
Edit: Yes, see https://pnpm.io/catalogs#publishing
If we do this, I'm slightly inclined to do it to all of our packages. |
It would be good to include only the dependencies that are repeated across packages, we don’t want |
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.
https://pnpm.io/catalogs#caveats
will these dep be updated by dependabot?
# multiple packages in the monorepo. | ||
# This allows us to manage versions and updates in one place. | ||
# https://pnpm.io/en/catalogs | ||
|
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.
@@ -106,7 +106,7 @@ | |||
"stylelint-order": "7.0.0", | |||
"stylelint-selector-bem-pattern": "4.0.1", | |||
"tsx": "^4.19.3", | |||
"typescript": "~5.8.2", | |||
"typescript": "catalog:", | |||
"typescript-eslint": "~8.31.1", |
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.
maybe add typescript-eslint
?
"@types/node": "22.15.3", | ||
"@types/react": "^19.1.0", | ||
"@types/node": "catalog:", | ||
"@types/react": "catalog:", | ||
"cross-env": "^7.0.3", |
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.
We should add cross-env
"cross-env": "^7.0.3", | ||
"css-loader": "~7.1.2", | ||
"eslint-plugin-react": "~7.37.4", | ||
"eslint-plugin-storybook": "~0.12.0", | ||
"global-jsdom": "^26.0.0", | ||
"postcss-loader": "~8.1.1", | ||
"react": "^19.1.0", | ||
"react": "catalog:", | ||
"storybook": "^8.6.11", | ||
"style-loader": "~4.0.0", | ||
"stylelint": "^16.19.1", | ||
"stylelint-config-standard": "^38.0.0", | ||
"stylelint-order": "7.0.0", | ||
"stylelint-selector-bem-pattern": "4.0.1", | ||
"tsx": "^4.19.3", |
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.
Maybe we should add our testing deps, like tsx JSdom, etc?
That also sounds good, but I don't think we should selectively include, I feel we should either: But that's just me |
Very much as an aside, but I am surprised none of the changes in this PR require approval from web-infra as a code owner. |
Is this catalog: syntax compatible with npm? Is this a more-or-less standard field in package.json? |
Agreed. These sort of changes should require web-infra as per the governance, they're not specifically related as a website development feature but more an infrastructural aspect. |
It relies on |
When we run |
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.
Appreciate the effort, @bjohansebas but I'm -1 on features that are locked on a specific package manager.
With all the respect in the world, this feels like a ridiculous reason to object when we 1. voted to use pnpm, and 2. are specifically enforcing that pnpm is used when contributing: Lines 54 to 59 in ed840c2
|
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 agree with @ovflowd — this change introduces an additional layer of complexity that contributors will need to learn, which could make contributing harder.
Also, it’s worth pointing out that if the consumer is importing via git subdirectories, this approach requires them to use pnpm, ruling out alternatives like yarn that also support git subdirectory imports.
Out of curiosity, we already use |
I expressed this in Slack, but honestly, I'm quite disappointed with the objections here, both because we actively voted to use pnpm yet are now against using the niceties it gives us, but also because I don't see how this is particularly anymore complex than the |
We intentionally avoid using the |
I recall our votes were explicitly to use pnpm in favor for our CI/CD and improve installation speed. I don't think we voted on using pnpm specific features that are only available for pnpm. I did not recall the PR that added pnpm-workspaces.yml and I'm pretty sure pnpm is compatible with npm workspaces. (https://docs.npmjs.com/cli/v7/using-npm/workspaces) I also feel we should use syntax as much as possible that is cross-compatible with other package managers. Although yes, users will be asked to use a) i should be able to install via npm or yarn or deno or bun or whatever I want But I want to make it explicit that our vote fas in favor of using There's a Collaboration Space within OpenJS with the specific goal of making Package Managers Interoperable; I feel dubious that whilst OpenJS is working towards that (ie @Ethan-Arrowood) we are doing the opposite by locking down our repo to only work with pnpm. |
We added that in the initial migration, IIRC. I didn't know pnpm supported npm workspaces. For |
I recall that yes; But didn't realize
As far as I know pnpm, yarn and npm support the simplified |
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.
FYI I don't want to block the PR unnecessarily, and apparently pnpm
does NOT support NPM's workspace
... and we're already using yarn/pnpm specific syntax with workspace:*
so let's gon ahead with this PR.
I still am -1 on this, but I am the minority, so I don't want to block this PR when everyone else is okay 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.
Why not but catalog is a new npm feature so idk if it's good idea
Description
Using a catalog allows you to define a dependency in a single place and share it with the rest of the packages that require it. This can be useful to ensure that the same dependencies are always shared consistently
Validation
Related Issues
https://openjs-foundation.slack.com/archives/CVAMEJ4UV/p1748813652063899
Check List
pnpm format
to ensure the code follows the style guide.pnpm test
to check if all tests are passing.pnpm build
to check if the website builds without errors.