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.

Sign up for GitHub

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

Jump to bottom

Pnpm9 #23318

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

Pnpm9 #23318

wants to merge 26 commits into from

Conversation

CraigMacomber
Copy link
Contributor

@CraigMacomber CraigMacomber commented Dec 12, 2024 •
edited
Loading

Description

Update to pnpm 9

Same as #21328 but not from a fork to work around issues with how dependency scanning works for PRs.

Breaking Changes

Developers will have to update to pnpm 9

Reviewer Guidance

The review process is outlined on this wiki page.

CraigMacomber and others added 13 commits June 4, 2024 10:00
@CraigMacomber
Update client to pnpm 9
6091a1d
@CraigMacomber
Make version 9.9.9 so bug repro will be stable across publishing RC5
737ac14
@KSXGitHub
chore: enable link-workspace-packages
f9a97ca 
Fixes pnpm/pnpm#8172
@CraigMacomber
Merge pull request #4 from KSXGitHub/patch-1
1d9f7cc 
chore: enable link-workspace-packages
@CraigMacomber
Revert "Make version 9.9.9 so bug repro will be stable across publish…
f48a188 
…ing RC5"

This reverts commit 737ac14.
@CraigMacomber
Update lock file
241cd57
@CraigMacomber
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
08b812d 
…to Pnpm9
@CraigMacomber
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
1081000 
…to Pnpm9
@CraigMacomber
Update lock file
ad97ade
@CraigMacomber
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
097cad6 
…to Pnpm9
@CraigMacomber
Update lock file
14edb2b
@CraigMacomber
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
512234c 
…to Pnpm9
@CraigMacomber
Update lock file
40ac441
@github-actions github-actions bot added dependencies Pull requests that update a dependency file base: main PRs targeted against main branch labels Dec 12, 2024
msfluid-bot
msfluid-bot reviewed Dec 13, 2024
View reviewed changes
Copy link
Collaborator

@msfluid-bot msfluid-bot left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code Coverage Summary

No packages impacted by the change.

Baseline commit: e8762e3
Baseline build: 312423
Happy Coding!!

Code coverage comparison check passed!!

@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Dec 13, 2024 •
edited
Loading

⯅ @fluid-example/bundle-size-tests: +245 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 469.94 KB 469.97 KB ⯅ +35 Bytes
azureClient.js 567 KB 567.05 KB ⯅ +49 Bytes
connectionState.js 724 Bytes 724 Bytes ■ No change
containerRuntime.js 266.11 KB 266.12 KB ⯅ +14 Bytes
fluidFramework.js 434.95 KB 434.96 KB ⯅ +14 Bytes
loader.js 134.22 KB 134.23 KB ⯅ +14 Bytes
map.js 42.72 KB 42.73 KB ⯅ +7 Bytes
matrix.js 150.17 KB 150.18 KB ⯅ +7 Bytes
odspClient.js 534.46 KB 534.51 KB ⯅ +49 Bytes
odspDriver.js 99.48 KB 99.5 KB ⯅ +21 Bytes
odspPrefetchSnapshot.js 43.04 KB 43.05 KB ⯅ +14 Bytes
sharedString.js 166.25 KB 166.25 KB ⯅ +7 Bytes
sharedTree.js 425.42 KB 425.43 KB ⯅ +7 Bytes
Total Size 3.41 MB 3.41 MB ⯅ +245 Bytes

Baseline commit: e8762e3

Generated by 🚫 dangerJS against 7bd8380

@github-actions github-actions bot added area: build Build related issues area: server Server related issues (routerlicious) area: website labels Dec 13, 2024
@CraigMacomber CraigMacomber marked this pull request as ready for review December 13, 2024 18:32
CraigMacomber
CraigMacomber commented Dec 13, 2024
View reviewed changes
.npmrc
@@ -1,11 +1,10 @@
engine-strict=true
frozen-lockfile=true
strict-peer-dependencies=true
link-workspace-packages=true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes to pnpm defaults require this for workspace deps to function during install now.

CraigMacomber
CraigMacomber commented Dec 13, 2024
View reviewed changes
.npmrc

# Disable pnpm update notifications since we use corepack to install package managers
update-notifier=false
# Don't use new lockfile format because component governance does not yet support it
use-lockfile-v6=false
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 don't think this did anything even before this change as I don't think pnpm 8 looked for this option. Regardless we are moving to format 9 now, so its out of date.

CraigMacomber
CraigMacomber commented Dec 13, 2024
View reviewed changes
server/gitrest/Dockerfile
@@ -15,6 +15,7 @@ WORKDIR /home/node/server
COPY package*.json ./
COPY pnpm*.yaml ./
COPY scripts/*.* ./scripts/
COPY .npmrc ./
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 don't know docker well, and couldn't reproduce the error CI hit locally, so this "fix" should get extra careful review.

@github-actions GitHub Actions
Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170006 links
    1595 destination URLs
    1825 URLs ignored
       0 warnings
       0 errors

CraigMacomber
CraigMacomber commented Dec 13, 2024
View reviewed changes
server/gitrest/Dockerfile
@@ -24,7 +25,8 @@ RUN corepack enable
# Need to set the --unsafe-perm flag since we are doing the install as root. Consider adding an 'app' accout so we
# can do the install as node but then switch to 'app' to run. As app we won't be able to write to installed files
# and be able to change them.
RUN pnpm install --unsafe-perm
# Needs to set --no-frozen-lockfile since npmrc requires it if there are resolution changes, and the package dependencies don't exactly match
RUN pnpm install --unsafe-perm --no-frozen-lockfile
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is bad, and should not be needed. Once #23330 is unblocked, and merged, this hack should be removed, then this PR can go forward

jason-ha
jason-ha reviewed Dec 19, 2024
View reviewed changes
Copy link
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

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

Can't really comment on Docker, but I love this.
Hopefully it is unblocked soon.

common/build/build-common/.npmrc
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have link-workspace-packages=true for consistency across our spaces? Or will we be trying to get rid of that in the other places?

server/routerlicious/Dockerfile
COPY pnpm*.yaml ./
COPY pnpm*.yaml ./s
Copy link
Contributor

Choose a reason for hiding this comment

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

Intentional? If so, comment please.

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

@msfluid-bot msfluid-bot msfluid-bot left review comments

@jason-ha jason-ha jason-ha left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area: build Build related issues area: server Server related issues (routerlicious) area: website base: main PRs targeted against main branch dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@CraigMacomber @msfluid-bot @jason-ha @KSXGitHub