-
Notifications
You must be signed in to change notification settings - Fork 138
chore: migrate to PNPM #1793
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
base: main
Are you sure you want to change the base?
chore: migrate to PNPM #1793
Conversation
cd61082 to
d5cd111
Compare
d5cd111 to
f4b7186
Compare
5939bb0 to
4bb73c4
Compare
| features-tests: | ||
| name: Features Tests | ||
| uses: temporalio/features/.github/workflows/typescript.yaml@main | ||
| uses: temporalio/features/.github/workflows/typescript.yaml@olszewski/pnpm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used to verify CI setup. Once both PRs are approved will switch to main before landing
| shell: bash | ||
| run: | | ||
| npm ci --ignore-scripts --verbose || npm ci --ignore-scripts --verbose || npm ci --ignore-scripts --verbose | ||
| pnpm install --frozen-lockfile || pnpm install --frozen-lockfile || pnpm install --frozen-lockfile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure about this, but it seems reasonable to hope that pnpm will be more stable and not require this "retry 3 times" hack.
| # End samples | ||
|
|
||
| # TODO: PNPM doesn't write logs to file by default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't mind just removing that step.
| { | ||
| "version": "1.13.2", | ||
| "npmClient": "npm", | ||
| "npmClient": "pnpm", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect the diff to show the previous line being deleted. Could it be that you duplicated the line rather than modify it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still some scripts in this file that use npm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some scripts here are still using npm.
| - 'packages/common' | ||
| - 'packages/core-bridge' | ||
| - 'packages/create-project' | ||
| - 'packages/docs' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what to do with the docs package. It was previously handled in isolation both to avoid conflicting dependencies and to speed up loading dependencies from an empty cache. PNPM may certainly help on both of those aspects, but I'm not convinced that that will be sufficient.
Could we consider removing docs from this workspace, and add a distinct pnpm-workspace.yaml file inside of docs?
What was changed
Move from NPM to PNPM package manager
Why?
PNPM offers a variety of improvements over NPM. Specifically, we're interested in the security benefits of minimumReleaseAge and not running dependency lifecycle scripts unless allowlisted.
Needs to be landed in conjunction with temporalio/features#698
Checklist
Closes N/A
How was this tested:
CI
Any docs updates needed?
N/A