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.

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

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

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.

@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
Copy link
Collaborator

@msfluid-bot msfluid-bot left a comment

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

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


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

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

Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] 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"


> [email protected] serve
> docusaurus serve --no-open

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

> [email protected] 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


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

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.

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?

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

Successfully merging this pull request may close these issues.

4 participants