chore(infra): migrate to pnpm#428
Conversation
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Sean Perkins <13732623+sean-perkins@users.noreply.github.com>
Updates many outdated dependencies and configurations.
Was removed in Lerna v7
tanner-reits
left a comment
There was a problem hiding this comment.
LGTM overall! Ran into one issue testing that's an easy fix.
...ages/example-project/component-library-angular/src/directives/angular-component-lib/utils.ts
Show resolved
Hide resolved
tanner-reits
left a comment
There was a problem hiding this comment.
Woops. Meant to request changes for that little fix, my b
christian-bromann
left a comment
There was a problem hiding this comment.
Two non blocking nits, overall LGTM 👍
|
Taking a look now 👀 |
| shell: bash | ||
| working-directory: ${{ inputs.working-directory }} | ||
| - name: Update Version | ||
| run: npx lerna version ${{ inputs.version }} --yes --exact --no-changelog --no-git-tag-version --preid=${{ inputs.preid }} |
There was a problem hiding this comment.
For my own understanding, is there a reason that we don't use pnpm version here?
I'm not super well versed in pnpm, so my understanding here might be off - IIUC, pnpm itself doesn't have a version command of its own, but does fall back to npm in some cases. If that's the case, I'm guessing there's a reason we use lerna to bump the version here instead of npm (but I can't remember for the life of me what it is).
There was a problem hiding this comment.
Personally I have not used pnpm version or was aware if it versions similar to lerna or falls back to it. If the team is interested in that, that is something that could continued to be investigated in further work. To leave as much of the infrastructure in place I would say keeping lerna provides confidence that versioning and the dev & prod release workflows continue to operate as they do today (and similar to other projects the team maintains, i.e.: Ionic Framework).
There was a problem hiding this comment.
That's fine - I was just trying to understand what lerna did/brought to the table here. Unfortunately, I think the pnpm/lerna knowledge gap is one the Framework + Stencil teams will have to bridge (moreso the Stencil team with respect to lerna, as that team doesn't use lerna in any of its projects) - with that in mind, I was trying (and I suppose my goal still is) to get a baseline for why we do certain things the way we do today, since figuring them out after some period of time is often more trying in my experience 😉
| "test": "jest", | ||
| "tsc": "tsc -p .", | ||
| "validate": "npm i && npm run lint && npm run test && npm run build" | ||
| "validate": "pnpm i && pnpm run lint && pnpm run test && pnpm run build" |
There was a problem hiding this comment.
This may fall in line with "we don't need this" (similar to some of my prev comments), but it might be good to explictly put --frozen-lockfile for if/when folks run this locally:
| "validate": "pnpm i && pnpm run lint && pnpm run test && pnpm run build" | |
| "validate": "pnpm i --frozen-lockfile && pnpm run lint && pnpm run test && pnpm run build" |
There was a problem hiding this comment.
I believe we can mark this as resolved too based on the outcome of: #428 (comment).
pnpm is discouraging explicit usage of the --frozen-lockfile CLI argument.
There was a problem hiding this comment.
sounds good to me!
packages/example-project/component-library-angular/src/directives/proxies.ts
Show resolved
Hide resolved
packages/example-project/component-library-vue/src/vue-component-lib/utils.ts
Show resolved
Hide resolved
| @@ -1,4 +1,4 @@ | |||
| import pkg from './package.json'; | |||
| import pkg from './package.json' assert { type: 'json' }; | |||
There was a problem hiding this comment.
Can we use with here? assert is deprecated per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules
There was a problem hiding this comment.
This would require Node.js v20.10 which requires us to update the Node version in package.json
|
It seems like we mostly have consensus to move forward with this. There are 3 remaining comments that we might want to address:
I don't have any strong preferences for the first two issues. In regards to the Volta comment I suggest to merge @rwaskiewicz suggestion noting that Volta support for PNPM is experimental and if you don't use it, be advised to have the right Node.js version installed. Thoughts @rwaskiewicz @sean-perkins ? |
|
Agreed with respect to Volta support - although we'll have to revisit our usage of https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#node-version-file in GH Actions if we remove it |
Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>
…gets into sp/pnpm
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally for affected output targetsnpm test) were run locally and passednpm run prettier) was run locally and passedPull request type
Please check the type of change your PR introduces:
What is the current behavior?
With the dependency on
@lit/reactthat adds a peer dependency for@types/react, the workspace has a type resolution issue when trying to reconcile the version of@types/reactin the example react project.What is the new behavior?
Infrastructure
Angular Output Target
Vue Output Target
Does this introduce a breaking change?
Other information
This PR is required to unblock #432.