Update root workspace to pnpm 11#27307
Conversation
| ╚══════════════════════════════════════════════════════════════════╝ | ||
| `; | ||
|
|
||
| const used_pnpm = process.env.npm_config_user_agent.startsWith(`pnpm`); |
There was a problem hiding this comment.
This crashed with pnpm 11 since npm_config_user_agent was undefined.
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (324 lines, 10 files), I've queued these reviewers:
How this works
|
|
I've seen a decent amount of patching of v11 for unexpected breaks, enough that it makes me want to wait another couple of weeks to give it time to bake before we use it. |
jason-ha
left a comment
jason-ha
left a comment
There was a problem hiding this comment.
I love moving forward and getting the pnpm settings out of package.json so comments can be coupled.
If we do partial move, can multiple workspaces be worked on within the same enlistment? Or is there a pnpm switcher like nvm OR does corepack manage that automagically?
| patchedDependencies: | ||
| '@microsoft/api-extractor@7.58.1': patches/@microsoft__api-extractor@7.58.1.patch | ||
| # engineStrict: true | ||
| frozenLockfile: true | ||
| strictPeerDependencies: true | ||
| linkWorkspacePackages: true | ||
| # Disable pnpm update notifications since we use corepack to install package managers | ||
| updateNotifier: false | ||
| # Use the number of cores on the machine by default. | ||
| workspaceConcurrency: 0 |
There was a problem hiding this comment.
Can we add some blank lines above the top-level sections to improve readability?
| # | ||
| # Guidelines for updating this file: | ||
| # | ||
| # 1. Keep the entries alphabetical wherever possible. |
There was a problem hiding this comment.
Is this comment meant to apply to top-level entries as well? I suspect that it may be good to have sections of top-level entries; so, these guidelines could be updated. Otherwise, at least sort the new content.
| - '@aws-sdk/*' | ||
| patchedDependencies: | ||
| '@microsoft/api-extractor@7.58.1': patches/@microsoft__api-extractor@7.58.1.patch | ||
| # engineStrict: true |
There was a problem hiding this comment.
What is up with engineStrict?
I see that it was present in .npmrc.
There was a problem hiding this comment.
We had a bunch of at least some what redundant systems for enforcing what tools we use (package manager and node version). As I removed engines in favor of devEngines, this had no effect. Unfortunately, as far as I can tell now that I test it, devEngines is the right thing to use, except it doesn't work, so I put back the engines workaround and restored engines strict here.
There was a problem hiding this comment.
Would we ever want this checked in again? If not, then can we add it to .gitignore? If we do that, then there may be some git status checks that can be cleaned up - currently it is common for changed files but exclude .npmrc in pipelines as it has temp auth tokens.
| "ci:build": "fluid-build --task ci:build", | ||
| "ci:build:docs": "fluid-build --task ci:build:docs", | ||
| "ci:check:are-the-types-wrong": "pnpm run -r --parallel --no-bail --color check:are-the-types-wrong", | ||
| "ci:check:are-the-types-wrong": "pnpm run -r --parallel --no-bail --color=auto check:are-the-types-wrong", |
There was a problem hiding this comment.
Okay, but what is going on?
Add note to PR description?
There was a problem hiding this comment.
color takes an argument now, so this was parsing incorrectly.
There was a problem hiding this comment.
I guess this was trying to opt in, so it now sets it to always.
…vEngines is not working.
3 participants
Description
Update root workspace to pnpm 11.
This is mostly based on the instructions in https://pnpm.io/migration
There was an additional fixup to deal with the trust policy exceptions for semver, see comment on it.
only-pnpm.cjs was crashing as well, so it was updated, and one of the 5 copies of it deduplicated.
The package.json file was migrated to use devEngines, but the pnpm entry in it was omitted as there does not seem to be a way to preserve that and keep the hash.
Note that npm will crash if used to install in this workspace (due to the workspace deps), and yarn will refuse to run due to the packageManager entry, so the only-pnpm script isn't really doing anything for this workspace.
It was kept as a bit of a redundent measure, and to be consistent with workspaces where it does matter.
That script was also reused from the compat-workspaces/full/package.json instead of duplicated.
Since this update was somewhat complex (a lot more changes than most pnpm updates), a limited change with just the root workspace is being done first.
Reviewer Guidance
The review process is outlined on this wiki page.