-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
chore: migrate to pnpm #11248
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?
chore: migrate to pnpm #11248
Conversation
refactor: removed .yarn folder fix: changed overrides approach feat: changed references
Replacing variables
Migrate scripts to pnpm and replaced yarn reference
Replaced yarn reference with pnpm
Added rule schema types
Fixed types
Launching app pnpm
Resolved problems with babel types
docs: updated comments to reflect pnpm usage instead of yarn
Hi @JoshuaKGoldberg, @JamesHenry, @bradzacher, @kirkwaiblinger, and the rest of the team! 👋 First of all, thank you for all the work you do maintaining this project — it’s a huge help to the community, and I really appreciate the time and care you put into it. I was just wondering if there’s any chance a maintainer could take a look at this PR. I understand you're all busy, but since it hasn’t received a review from a maintainer yet, I’d be happy, along with @Jester175, to revise or adjust anything needed once there’s some feedback. Thanks again! |
# Conflicts: # tools/scripts/generate-sponsors.mts # yarn.lock
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.
Thanks for this @xaos7991!
Please kindly take a look at all the inline comments.
Additionally:
-
The changes to the dependency and task graphs of the workspace through this change of package manager are unexpected and make me nervous that scope creep has occurred. Can we achieve this change of package manager without changing the relationships in the repo please? If they need to be changed for correctness let's please discuss and potentially address in a follow up (or pre-requisite PR)
-
I am not familiar enough with
-w exec
so I will need to check out the PR to play with that in all the subpackages, which I will do when other tasks have been addressed. -
I will push a commit to change the release setup once all other TODOs are addressed and the PR is green again
.github/workflows/nx-migrate.yml
Outdated
|
||
YARN_ENABLE_IMMUTABLE_INSTALLS=false yarn install | ||
pnpm install |
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.
The install should allow modifications:
# We need to expect lock file changes to be applicable
pnpm install --ignore-scripts --frozen-lockfile=false
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.
Done
.github/workflows/nx-migrate.yml
Outdated
|
||
# Sometimes Nx can require config formatting changes after a migrate command | ||
YARN_ENABLE_IMMUTABLE_INSTALLS=false yarn install | ||
yarn nx format | ||
pnpm install |
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 again
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.
Done
@@ -43,12 +43,12 @@ const { projectsVersionData, workspaceVersion } = await releaseVersion({ | |||
|
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.
This needs to be redone, you can leave it to me
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.
Thank you so much!
It would be great if you could take care of that part - totally trust your judgment on it. Let me know if there's anything you'd like me to adjust on my side!
During our work, we discovered that specifying a version like: However, referencing it as a workspace: It seems this package isn't published publicly and is meant to be used internally within the monorepo. Might be worth clarifying to avoid confusion. |
Uh oh! @Jester175, at least one image you shared is missing helpful alt text. Check #11248 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
@@ -20,10 +20,11 @@ jobs: | |||
name: Run prettier formatting if required | |||
runs-on: ubuntu-latest | |||
steps: | |||
- uses: actions/checkout@v4 | |||
- uses: pnpm/action-setup@v4 | |||
if: ${{ steps.nrwl-workspace-package-check.outputs.was-changed == 'true' }} |
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.
Wrong condition ID reference and wrong place in the workflow (per previous 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.
Done
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's also completely unchanged...
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.
Ok sorry, looks like there was a github caching issue. It is now in the correct place, but the step ID is still wrong
if: ${{ steps.nrwl-workspace-package-check.outputs.was-changed == 'true' }} | |
if: ${{ steps.prettier-package-check.outputs.was-changed == 'true' }} |
"resolutions": { | ||
"@types/eslint-scope": "link:./tools/dummypkg", | ||
"@types/eslint": "link:./tools/dummypkg", | ||
"@types/estree": "link:./tools/dummypkg", | ||
"@types/node": "^22.0.0", | ||
"@types/react": "^18.2.14", | ||
"eslint-plugin-eslint-plugin@^5.5.0": "patch:eslint-plugin-eslint-plugin@npm%3A5.5.1#./.yarn/patches/eslint-plugin-eslint-plugin-npm-5.5.1-4206c2506d.patch", | ||
"prettier": "3.5.0", | ||
"react-split-pane@^0.1.92": "patch:react-split-pane@npm%3A0.1.92#./.yarn/patches/react-split-pane-npm-0.1.92-93dbf51dff.patch", | ||
"tsx": "^4.7.2", | ||
"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.
I don't think these can casually be removed and leave things to "*" references in the dependences/devDependencies.
Maybe @bradzacher can give feedback on the specifics
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.
Oh sorry, I see the overrides config in pnpm-workspace.yaml
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'd still be curious to get sign off from Brad on the whole "link:./tools/dummypkg" thing, I can't remember off the top of my head what that is about
That's because in pnpm 10+ you have to opt into it linking workspace packages by version number reference: https://pnpm.io/cli/recursive#--link-workspace-packages It's totally fine that we are switching to It's not replacing a version number reference. Again, if these are required, and somehow yarn was causing us to miss them, that's one thing, but I want to make sure we are clearly explaining why this was necessary in each case |
Uh oh! @JamesHenry, at least one image you shared is missing helpful alt text. Check #11248 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
# Conflicts: # packages/eslint-plugin/package.json # packages/type-utils/package.json # yarn.lock
The addition of Also added
![]() |
Uh oh! @xaos7991, at least one image you shared is missing helpful alt text. Check #11248 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
# Conflicts: # yarn.lock
PR Checklist
Overview