withastro / action

A GitHub Action that deploys your Astro project to GitHub Pages
Other
143 stars 31 forks source link

Add option to configure pnpm version #25

Closed ThatXliner closed 10 months ago

ThatXliner commented 11 months ago
wouter173 commented 11 months ago

I have encountered issues with the workflow only supporting pnpm 7. This would fix them, Please merge!

natemoo-re commented 11 months ago

I would almost rather remove the with: version option from the pnpm action and require a packageManager field in the package.json.

What do we think?

SelfhostedPro commented 11 months ago

I would almost rather remove the with: version option from the pnpm action and require a packageManager field in the package.json.

What do we think?

It would be better to have the option in the action in order to allow more flexibility on the CI side. Just my 2 cents.

ThatXliner commented 11 months ago

I would almost rather remove the with: version option from the pnpm action and require a packageManager field in the package.json.

What do we think?

IIRC that’s a corepack feature which didn’t work on my computer. Not everyone can use corepack.

Thanks for the PR @ThatXliner! Could you also add pnpm-version to the documentation for the action inputs in the README and to the commented-out with examples in the example workflow there?

Done. Can you check my formatting?

ThatXliner commented 11 months ago

Docs LGTM!

I guess one option is that we don’t set a default pnpm-version, so if it isn’t set, the package.json field will be picked up, but if it is set we override that? That’s how https://github.com/pnpm/action-setup/ works too I believe.

That would break existing CIs (although that being said, since I made the default latest, technically that would break compatibility as well)

lorenzolewis commented 11 months ago

That would break existing CIs (although that being said, since I made the default latest, technically that would break compatibility as well)

I wonder if instead of latest as the default it instead stays with 7.x.x. That way it doesn't break any existing setups but still exposes the option to override it and get workflows up and running.

ThatXliner commented 11 months ago

yea I'll change that

delucis commented 11 months ago

That would break existing CIs (although that being said, since I made the default latest, technically that would break compatibility as well)

Can’t we just release this as a major version? That way existing users get the current behaviour until they intentionally update.

ThatXliner commented 11 months ago

Can’t we just release this as a major version? That way existing users get the current behaviour until they intentionally update.

That seems like a good idea. Not sure about community consensus (unless none is required).

delucis commented 11 months ago

I’ll let @natemoo-re take the final decision, but this change seems to unblock people and make the action more flexible, so seems worth releasing a v1 to me! 👍

NicoToff commented 10 months ago

Hi guys, any working fix on this issue?

DigitalNaut commented 10 months ago

I'm also eagerly awaiting this upgrade, it'd be great to have! 🙏

ThatXliner commented 10 months ago

The PR is currently ready for merging w/o a major version bump (as the default version is the same as before).

In the future, changing the default to be empty should be considered because that way it will match the behavior of setup-pnpm, but this will require bumping the semver major version.

Until this gets merged, an easy workaround is to implement this action manually: https://github.com/ThatXliner/blog/commit/d11859d65d9aa15b2be87e48f8580d4a9c465488#diff-28802fbf11c83a2eee09623fb192785e7ca92a3f40602a517c011b947a1822d3

DigitalNaut commented 10 months ago

The PR is currently ready for merging w/o a major upgrade (as the default version is the same as before).

In the future, changing the default to be empty should be considered because that way it will match the behavior of setup-pnpm, but this will require bumping he semver major version.

Until this gets merged, an easy workaround is to implement this action manually: ThatXliner/blog@d11859d#diff-28802fbf11c83a2eee09623fb192785e7ca92a3f40602a517c011b947a1822d3

Thank you so much! I'm not very familiar with GH Actions yet but this helps a lot