Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
xaos7991 wants to merge101 commits intotypescript-eslint:main
base:main
Choose a base branch
Loading
fromxaos7991:migrate-to-pnpm

Conversation

xaos7991
Copy link
Contributor

@xaos7991xaos7991 commentedMay 27, 2025•
edited
Loading

PR Checklist

Overview

JoshuaKGoldberg, benmccann, lishaduck, and kirkwaiblinger reacted with thumbs up emojiJounQin reacted with thumbs down emojibenmccann and lishaduck reacted with heart emoji
xaos7991and others added30 commitsMay 18, 2025 21:32
refactor: removed .yarn folderfix: changed overrides approachfeat: 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
@xaos7991
Copy link
ContributorAuthor

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 and JoshuaKGoldberg reacted with thumbs up emoji

@kirkwaiblingerkirkwaiblinger requested a review froma teamJuly 6, 2025 22:22
Copy link
Member

@JamesHenryJamesHenry 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


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 applicablepnpm install --ignore-scripts --frozen-lockfile=false

Jester175 reacted with thumbs up emoji

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

Jester175 reacted with thumbs up emoji

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 commentedJul 9, 2025•
edited
Loading

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.
404 error when installing @typescript-eslint/rule-schema-to-typescript-types with ^8.36.0, works with workspace:*

@github-actionsGitHub Actions
Copy link

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 atBasic writing and formatting syntax: images on GitHub Docs.

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

@xaos7991xaos7991 requested a review fromJamesHenryJuly 9, 2025 19:23
@@ -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' }}
Copy link
Member

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)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Done

Copy link
Member

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

Copy link
Member

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

Suggested change
if:${{ steps.nrwl-workspace-package-check.outputs.was-changed == 'true' }}
if:${{ steps.prettier-package-check.outputs.was-changed == 'true' }}

Comment on lines -107 to -118
"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"
},
Copy link
Member

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

Copy link
Member

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 inpnpm-workspace.yaml

Copy link
Member

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

@JamesHenry
Copy link
Member

@Jester175

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.

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 toworkspace:* references (that's a desirable thing as part of this move), but I'm still just surprised that changes to the project and task relationships were required, e.g. you have altered a fewdependsOn and added new dependency references like
image

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

@github-actionsGitHub Actions
Copy link

Uh oh!@JamesHenry, 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 10

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 atBasic writing and formatting syntax: images on GitHub Docs.

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

# Conflicts:#packages/eslint-plugin/package.json#packages/type-utils/package.json#yarn.lock
@xaos7991
Copy link
ContributorAuthor

@Jester175

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.

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 toworkspace:* references (that's a desirable thing as part of this move), but I'm still just surprised that changes to the project and task relationships were required, e.g. you have altered a fewdependsOn and added new dependency references likeimage

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

The addition of@types/json-schema was necessary because TypeScript was complaining about missing type declarations when using the json-schema package inareOptionsValid.ts:
image

Also added@types/react — a few tests were relying on React types and started failing without them.
Falling tests:

image

@github-actionsGitHub Actions
Copy link

Uh oh!@xaos7991, 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 9
  • Images should have meaningful alternative text (alt text) at line 16
  • Images should have meaningful alternative text (alt text) at line 25

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 atBasic writing and formatting syntax: images on GitHub Docs.

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@benmccannbenmccannbenmccann left review comments

@Jester175Jester175Jester175 left review comments

@aryaemami59aryaemami59aryaemami59 left review comments

@JamesHenryJamesHenryAwaiting requested review from JamesHenry

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Repo: Migrate from yarn to pnpm
6 participants
@xaos7991@JoshuaKGoldberg@Jester175@JamesHenry@benmccann@aryaemami59
[8]ページ先頭
©2009-2025 Movatter.jp