Skip to content

chore: Upgrade from yarn classic (v1.22) to pnpm v10#66

Merged
bantic merged 32 commits intomasterfrom
bantic/switch-to-pnpm
Jan 14, 2025
Merged

chore: Upgrade from yarn classic (v1.22) to pnpm v10#66
bantic merged 32 commits intomasterfrom
bantic/switch-to-pnpm

Conversation

@bantic
Copy link
Contributor

@bantic bantic commented Jan 10, 2025

Upgrades from yarn classic as a package-manager to pnpm.

Followed guidance from https://dev.to/andreychernykh/yarn-npm-to-pnpm-migration-guide-2n04 and https://pnpm.io/installation.

In some cases I referred to existing v2 addons w/ pnpm (like ember-page-title) as a reference.
Also looked at https://github.com/embroider-build/addon-blueprint.

pnpm surfaced some mistakes where dependencies hadn't been correctly defined (but were hoisted when using yarn classic). It also complains more loudly about deprecated deps which led me to upgrade eslint 8 to 9 for addon/.

This PR diverges a little from the github workflow guidance from pnpm (https://pnpm.io/continuous-integration#github-actions) in that it uses volta's cli action instead of setup-node.

This change sets up this repo so that:

  • locally:
    • node version is managed by volta (via the top-level volta.node in package.json)
    • pnpm version is managed by corepack (via top-level packageManager in package.json)
  • at CI:
    • volta-cli action used, which reads volta.node from package.json and sets up node
    • setup-pnpm action used, which reads packageManager from package.json and sets up pnpm

This uses actions/cache directly, and adds an extra step to each job to find the pnpm "store" path to use for the cache. I determined how to do this by looking at how setup-node does it when cache: 'pnpm' is used (https://github.com/actions/setup-node/blob/48b90677b6048efbc723b11a94acb950d3f1ac36/src/cache-utils.ts#L30-L38), and found some examples in the wild like: https://gist.github.com/belgattitude/838b2eba30c324f1f0033a797bab2e31.

Other changes:

@bantic bantic force-pushed the bantic/switch-to-pnpm branch from f45dc26 to c17a0c5 Compare January 10, 2025 13:35
@bantic bantic force-pushed the bantic/switch-to-pnpm branch from 2a93c09 to e149125 Compare January 13, 2025 10:10
"ember-cli-sri": "^2.1.1",
"ember-cli-terser": "^4.0.2",
"ember-json-viewer": "*",
"ember-json-viewer": "workspace:*",
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 is the pnpm way to refer to a workspace package, see https://pnpm.io/workspaces#workspace-protocol-workspace

@@ -0,0 +1,115 @@
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to upgrade eslint 8 to 9 for the addon/ dir, copied with minor changes from test-app's eslint config

@@ -1,9 +1,9 @@
import babel from '@rollup/plugin-babel';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to .mjs to remove a deprecation warning from rollup.

The other changes are cosmetic, applied by prettier to fix linting.

"docs-app"
],
"scripts": {
"preinstall": "npx only-allow pnpm"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"start": "ember serve",
"test": "concurrently \"npm:lint\" \"npm:test:*\" --names \"lint,test:\" --prefixColors auto",
"test:ember": "ember test",
"test:ember-compatibility": "ember try:each"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped this because we don't want it to be included when doing pnpm test.
We run the scenarios individually in the workflow

Comment on lines +36 to +37
"@embroider/compat": "^3.4.8",
"@embroider/core": "^3.4.8",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pnpm identified that these had been missing before (yarn classic had been hoisting them from elsewhere which made them available to the test-app. pnpm doesn't allow that, so they have to be explicitly added)

@bantic bantic changed the title [draft] Switch to pnpm chore: Upgrade from yarn classic (v1.22) to pnpm Jan 13, 2025
@bantic bantic marked this pull request as ready for review January 13, 2025 15:31
@bantic bantic requested a review from a team January 13, 2025 15:42
@bantic bantic changed the title chore: Upgrade from yarn classic (v1.22) to pnpm chore: Upgrade from yarn classic (v1.22) to pnpm v10 Jan 13, 2025
- name: Install Dependencies
if: steps.node-cache.outputs.cache-hit != 'true'
run: yarn --frozen-lockfile
run: pnpm install
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verified that the cache is working correctly by reviewing a recent run:

image

Copy link
Contributor

@eliasdawson-addepar eliasdawson-addepar left a comment

Choose a reason for hiding this comment

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

Is there a reason you prefer to use volta to manage pnpm version? @yanglinz's recommendation was to use corepack for pnpm management and only use Volta for node version.

@bantic bantic requested a review from a team January 13, 2025 16:31
@bantic
Copy link
Contributor Author

bantic commented Jan 13, 2025

Is there a reason you prefer to use volta to manage pnpm version?

I misunderstood that. I'll retool this once again to use volta only for node and use corepack for pnpm.

@bantic bantic marked this pull request as draft January 13, 2025 16:50
@bantic bantic force-pushed the bantic/switch-to-pnpm branch from 8c0bfe9 to d542b77 Compare January 13, 2025 16:56
@bantic bantic marked this pull request as ready for review January 13, 2025 17:03
@mixonic
Copy link
Member

mixonic commented Jan 13, 2025

I think this is on the right track now. Deferring to @yanglinz for alignment on the right timing to introduce corepack into an Addepar repo. I would doubt most people have it configured today, but with your approval that doesn't need to prevent us from starting to use it inside Web Core (likely the only people who will touch this code in the next few months).

@@ -20,40 +20,45 @@ jobs:
steps:
- uses: actions/checkout@v4
- uses: volta-cli/action@v4
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 sets up node, using the node version from package.json#volta.node

steps:
- uses: actions/checkout@v4
- uses: volta-cli/action@v4
- uses: pnpm/action-setup@v4
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 sets up pnpm, using the pnpm version from package.json#packageManager

@bantic bantic added this pull request to the merge queue Jan 14, 2025
Merged via the queue into master with commit f2275f3 Jan 14, 2025
16 checks passed
@bantic bantic deleted the bantic/switch-to-pnpm branch January 14, 2025 15:10
@github-actions github-actions bot mentioned this pull request Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

update contributor docs to reflect v2 addon format

4 participants