Skip to content

Conversation

dominikg
Copy link
Member

No description provided.

@dominikg
Copy link
Member Author

looks like the file: protocol in for _test_dependencies works differently in pnpm@8 :/

bluwy
bluwy previously approved these changes Apr 14, 2023
@bluwy
Copy link
Member

bluwy commented Apr 14, 2023

Ah wait I approved without reading the comments 🙈

@benmccann
Copy link
Member

Should we use pnpm/action-setup in the CI like SvelteKit does? I noticed this repo has some custom logic currently which could be simplified

Comment on lines -60 to -62
"pnpm": "^7.13.0",
"yarn": "forbidden, use pnpm",
"npm": "forbidden, use pnpm",
Copy link
Member

Choose a reason for hiding this comment

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

you can leave this in place if you'd like. you just need to switch ^ to >= which I believe is the thing that's currently stopping me from using this repo

Copy link
Member Author

Choose a reason for hiding this comment

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

you can enable corepack locally to ease working with different package manager versions, or switch manually. until the issue with 8 is resolved, 7 must be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

added back engines.pnpm and rewriting it for node14 tests. left the other two removed as neither npm nor yarn check it anymore i think.

@benmccann benmccann mentioned this pull request Apr 14, 2023
@dominikg
Copy link
Member Author

Should we use pnpm/action-setup in the CI like SvelteKit does? I noticed this repo has some custom logic currently which could be simplified

that action uses curlbashing and it also adds an additional point where pnpm version is set. this setup here uses packageManager as canonical and doesnt require a separate action. ymmv but i very much prefer the current approach

@benmccann
Copy link
Member

Just FYI, the action also uses packageManager and doesn't require setting the version used separately from that

@dominikg dominikg merged commit 89ea5e8 into main Apr 16, 2023
@dominikg dominikg deleted the chore/update-pnpm-8 branch April 16, 2023 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants