Skip to content

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

Open
wants to merge 95 commits into
base: main
Choose a base branch
from

Conversation

xaos7991
Copy link
Contributor

@xaos7991 xaos7991 commented May 27, 2025

PR Checklist

Overview

xaos7991 and others added 30 commits May 18, 2025 21:32
refactor: removed .yarn folder

fix: changed overrides approach

feat: changed references
Migrate scripts to pnpm and replaced yarn reference
Resolved problems with babel types
docs: updated comments to reflect pnpm usage instead of yarn
Jester175 and others added 2 commits June 16, 2025 15:27
# Conflicts:
#	package.json
#	packages/eslint-plugin/package.json
#	packages/type-utils/package.json
#	packages/website/package.json
#	yarn.lock
fix: resolved conflicts

fix: corrected the version
xaos7991 and others added 6 commits June 28, 2025 09:17
# Conflicts:
#	CONTRIBUTORS.md
#	packages/eslint-plugin/package.json
#	packages/type-utils/package.json
#	tools/scripts/generate-contributors.mts
#	yarn.lock
# Conflicts:
#	packages/eslint-plugin/package.json
#	packages/type-utils/package.json
#	yarn.lock
# Conflicts:
#	knip.ts
#	nx.json
#	packages/ast-spec/package.json
#	packages/eslint-plugin-internal/package.json
#	packages/eslint-plugin/package.json
#	packages/integration-tests/package.json
#	packages/parser/package.json
#	packages/rule-schema-to-typescript-types/package.json
#	packages/rule-tester/package.json
#	packages/scope-manager/package.json
#	packages/type-utils/package.json
#	packages/types/package.json
#	packages/typescript-eslint/package.json
#	packages/typescript-estree/package.json
#	packages/utils/package.json
#	packages/visitor-keys/package.json
#	yarn.lock
@xaos7991
Copy link
Contributor Author

xaos7991 commented Jul 1, 2025

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!

@kirkwaiblinger kirkwaiblinger requested a review from a team July 6, 2025 22:22
Copy link
Member

@JamesHenry JamesHenry left a 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

@@ -30,50 +30,24 @@ runs:
working-directory: ${{ inputs.working-directory }}
run: echo ${{ github.ref }}

- uses: actions/checkout@v4
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add another checkout here?

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 probably didn't notice that extra checkout. I've removed it now based on your suggestion.

@@ -129,7 +129,7 @@ jobs:
uses: ./.github/actions/prepare-build

- name: Run Check
run: yarn ${{ matrix.lint-task }}
run: pnpm ${{ matrix.lint-task }}
Copy link
Member

Choose a reason for hiding this comment

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

Missing run for consistency with your other ones

Choose a reason for hiding this comment

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

Hi! 👋
Thanks a lot for the review🙌

Just a quick note on this:
- name: Run Check
run: pnpm ${{ matrix.lint-task }}

You're right that for consistency with the other commands, it might make sense to use run: pnpm run .... However, in our case, we have one task in the matrix (knip) that directly runs a binary (i.e. knip) and doesn't currently have a corresponding script entry in package.json.

To use pnpm run knip, we'd need to define a script like "knip": "knip" in package.json. We chose to call the binary directly here to avoid extra script declarations.

Please correct me if I’m mistaken, but that was the reasoning behind the current setup. Happy to align if you think we should change it! 😊

Copy link
Member

Choose a reason for hiding this comment

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

pnpm needs to be installed into this workflow for it to continue to work, it should be done without running an install.

      - uses: pnpm/action-setup@v4
        if: ${{ steps.nrwl-workspace-package-check.outputs.was-changed == 'true' }}
        name: Install pnpm
        with:
          run_install: false

You can cross-reference with my other repo which already uses pnpm here: https://github.com/angular-eslint/angular-eslint/blob/main/.github/workflows/nx-migrate.yml

Choose a reason for hiding this comment

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

Done


YARN_ENABLE_IMMUTABLE_INSTALLS=false yarn install
pnpm install
Copy link
Member

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

Choose a reason for hiding this comment

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

Done


# Sometimes Nx can require config formatting changes after a migrate command
YARN_ENABLE_IMMUTABLE_INSTALLS=false yarn install
yarn nx format
pnpm install
Copy link
Member

Choose a reason for hiding this comment

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

Same again

Choose a reason for hiding this comment

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

Done

@@ -68,9 +68,6 @@
{
"runtime": "node -v"
},
{
"runtime": "yarn -v"
Copy link
Member

Choose a reason for hiding this comment

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

Replace with pnpm -v, don't remove

Choose a reason for hiding this comment

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

Done

@@ -1,5 +1,6 @@
{
"devDependencies": {
"eslint": "8.57.0"
"eslint": "8.57.0",
"@eslint/js": "latest"
Copy link
Member

Choose a reason for hiding this comment

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

This is a different major version to the eslint package, this feels wrong...

Choose a reason for hiding this comment

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

Done

@@ -35,7 +35,7 @@ export function eslintIntegrationTest(
let stderr = '';
try {
await execFile(
'yarn',
'pnpm',
Copy link
Member

Choose a reason for hiding this comment

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

Missing usage of run for consistency

Choose a reason for hiding this comment

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

Done

@@ -94,7 +94,7 @@ export function typescriptIntegrationTest(
): void {
integrationTest(testName, testFilename, async testFolder => {
const [result] = await Promise.allSettled([
execFile('yarn', ['tsc', '--noEmit', '--skipLibCheck', ...tscArgs], {
execFile('pnpm', ['tsc', '--noEmit', '--skipLibCheck', ...tscArgs], {
Copy link
Member

Choose a reason for hiding this comment

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

Missing usage of run for consistency

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({

Copy link
Member

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

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!

@Jester175
Copy link

Jester175 commented Jul 9, 2025

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

During our work, we discovered that specifying a version like: "@typescript-eslint/rule-schema-to-typescript-types": "^8.36.0" - results in a 404 error from the npm registry.

However, referencing it as a workspace: "@typescript-eslint/rule-schema-to-typescript-types": "workspace:*" - works as expected with pnpm.

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.
image

Copy link

github-actions bot commented Jul 9, 2025

Uh oh! @Jester175, at least one image you shared is missing helpful alt text. Check #11248 (comment) to fix the following violations:

  • Images should have meaningful alternative text (alt text) at line 16

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.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

@xaos7991 xaos7991 requested a review from JamesHenry July 9, 2025 19:23
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.

Repo: Migrate from yarn to pnpm
6 participants