-
Notifications
You must be signed in to change notification settings - Fork 351
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
Update example-basic-pnpm.yml #1291
Conversation
- Add hint how to omit setting the pnpm version explicitly - Add hint that `use-node-version` is not supported yet
|
|
Thank you for your submission to add comments to .github/workflows/example-basic-pnpm.yml! If you would like the Cypress.io team to consider your submission, you would need to sign the CLA agreement. In terms of the content, the workflow does already include links to the action repositories: The workflow comments are intended to support a minimal working example and not cover all variations. Details of the actions used can change over time and adding more detail brings in the risk that the added detail of external actions may become inaccurate over time. We rely on the external action documentation being kept up to date and for users to refer to that up to date documentation. My preference would be to not add this detail for the above reasons. You can wait to see if the Cypress.io team also responds. |
@@ -21,13 +21,17 @@ jobs: | |||
# See https://github.com/pnpm/action-setup | |||
- name: Install pnpm | |||
uses: pnpm/action-setup@v4 | |||
# Optional when there is a `packageManager` field in the `package.json`. | |||
# See https://github.com/pnpm/action-setup?tab=readme-ov-file#version |
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.
https://nodejs.org/api/corepack.html lists this feature as experimental and not suitable for production.
Stability: 1 - Experimental. The feature is not subject to semantic versioning rules. Non-backward compatible changes or removal may occur in any future release. Use of the feature is not recommended in production environments.
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.
Interesting. However since the action has documentation on supporting it, they will still lookup the information from there, even if packageManager
is not an official property anymore, right?
with: | ||
version: 9 | ||
|
||
# See https://github.com/actions/setup-node | ||
- name: Install Node.js | ||
uses: actions/setup-node@v4 | ||
with: | ||
# Note that pnpm `use-node-version` is not supported, yet | ||
# See https://github.com/actions/setup-node/issues/1130 |
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.
actions/setup-node#1130 is an open enhancement request
This isn't really the right place to be listing an enhancement request for an external action, when this isn't used in the example.
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.
My point was to link to some place that explains that there is no way to set the node version based on the config that pnpm supports. I don't see a better place where this is documented.
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 in-line comments also added.
Thanks. I understand if those code comments are not in line with what you want in your example. Feel free to close this PR in this case. |
Thanks for understanding that we try to keep the documentation as lean as possible for maintainability reasons. I'm going to close this PR now. Thank you nevertheless for your proposal! |
Having those hints inline saves quite a bit of research.
use-node-version
is not supported yet