Skip to content
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

Use pnpm + workspaces #3382

Closed
wants to merge 1 commit into from
Closed

Conversation

nickgros
Copy link
Contributor

@nickgros nickgros commented Jan 16, 2023

Reasons for making this change

pnpm + workspaces + Lerna deduplicates dependencies between packages. Only one copy of a dependency version will be installed across the entire repo. Only one lockfile is needed for the entire project. This should greatly reduce the amount of time needed to install dependencies, and improve overall developer experience.

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

package.json Outdated
Comment on lines 47 to 49
"resolutions": {
"@types/react": "^17.0.39"
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: I think I can remove this and just have this in utils/package.json

@@ -129,7 +129,7 @@ function AltDateWidget<

return (
<Box>
<Box display="flex" flexWrap="wrap" alignItems="center" justify="center">
<Flex flexWrap="wrap" alignItems="center">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I happened to pull in a more recent version of chakra-ui and discovered justify is not a valid prop on Box. We can use the Flex component, instead. I think we actually want to keep this left-aligned, though, so I ended up removing justify="center" anyways.

Comment on lines 18 to 24
"@mui/styles": path.resolve("../../node_modules", "@mui/styles"),
"@material-ui/styles": path.resolve(
"./node_modules",
"../../node_modules",
"@material-ui/styles"
),
"@emotion/react": path.resolve("./node_modules/@emotion/react"),
"@emotion/styled": path.resolve("./node_modules/@emotion/styled"),
"@emotion/react": path.resolve("../../node_modules/@emotion/react"),
"@emotion/styled": path.resolve("../../node_modules/@emotion/styled"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the dependencies are now deduplicated and stored in the root node_modules folder, we need to update these references.

Comment on lines 63 to 65
"resolutions": {
"@types/react": "^17.0.48"
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some dependency is pulling in @types/react@* which causes us to pull an incompatible version of @types/react, so let's force using v17.

Comment on lines 54 to 57
"resolutions": {
"ajv": "^8.11.0"
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: I think I added this trying to fix the old playground issue, and can remove this.

@cdaringe
Copy link

strong encouragement to check out pnpm workspaces. pnpm workspaces, based on my personal research, trump yarn workspaces by features/stability/performance. my yarn workspace experience is out-of-date by probably 8 months, but pnpm experience is fresh, and it's delightful.

@bollwyvl
Copy link
Contributor

👍 to yarn and (modern) lerna. I have no experience with pnpm, but also have very few complaints about the yarn 1.x experience, while 2.x+ seems to have gone wandering in the woods a bit.

One can go a little beyond the built-in yarn deduplication with yarn-deduplicate, which can squeeze a few more things out (especially when paired with a few tactical resolutions).

Also, on the typescript side: having a metapackage which contains references to all the other packages allows for having a single, incremental tsc -b entrypoint, which makes a big difference in both improving performance of type-checking at scale, and complexity.

@nickgros
Copy link
Contributor Author

@cdaringe @bollwyvl thanks for the perspectives! While I have more experience with yarn, pnpm ended up being a breeze to configure comparatively, so we'll go with pnpm for now.

If incremental builds end up being pretty easy to switch to, then I'll include that in this change, as well.

@nickgros nickgros changed the title Use yarn workspaces Use pnpm + workspaces Jan 27, 2023
@nickgros nickgros force-pushed the yarn-workspaces branch 4 times, most recently from 0d3ad26 to 10e40f7 Compare January 27, 2023 18:27
@ahkhanjani
Copy link
Contributor

Why not use Nx for distributed caching? Has the same benefits with way better tooling and will reduce lint, test and build time to potentially under 10 seconds

@nickgros
Copy link
Contributor Author

Why not use Nx for distributed caching? Has the same benefits with way better tooling and will reduce lint, test and build time to potentially under 10 seconds

I don't think that this change would preclude adding Nx cloud caching, which has no impact on dependency install time anyways.

@ahkhanjani
Copy link
Contributor

I don't think that this change would preclude adding Nx cloud caching, which has no impact on dependency install time anyways.

@nickgros But why use pnpm workspaces over Nx itself?

@cdaringe
Copy link

cdaringe commented Mar 13, 2023

As a nx user and a pnpm user and a pnpm+nx user, there are many reasons why would or wouldnt. Reasons to not would be high maintenance costs, poor debugging story, high noise:signal ratio of boilerplate, stringly typed references to workspace/project entities. There are good reasons as well (affected). Not sure nx is worth the effort for a project of this size. Were i the maintainer, Id opt out until something demanded it

@ahkhanjani
Copy link
Contributor

As a nx user and a pnpm user and a pnpm+nx user, there are many reasons why would or wouldnt. Reasons to not would be high maintenance costs, poor debugging story, high noise:signal ratio of boilerplate, stringly typed references to workspace/project entities. There are good reasons as well (affected). Not sure nx is worth the effort for a project of this size. Were i the maintainer, Id opt out until something demanded it

@cdaringe I think having internal libraries (validator-ajv6/8, utils, etc.) as a regular import and not a package, and most importantly "affected" will make the local and deployment process a hundred times faster at the very least. running all the tests and linting and then building every package for the change of only one package is something worth considering in my opinion.

Best regards

@ahkhanjani
Copy link
Contributor

Any updates on this?

@nickgros
Copy link
Contributor Author

@ahkhanjani I keep getting errors in CI that are hard to reproduce locally. Haven't had much time to try to find the root cause.

@ahkhanjani
Copy link
Contributor

@nickgros I will try to do the same thing and fix some issues. The problems seem to be poor typing issues in some places

@ahkhanjani ahkhanjani mentioned this pull request Apr 10, 2023
8 tasks
- Replace npm with pnpm
- Upgrade dependencies and include all directly used dependencies in package.json files
- Update chakra-ui AltDateWidget to use Flex instead of Box with flex property (fixes build issue)
- Update snapshots
- Update CONTRIBUTING to reference pnpm
- Update CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants