Skip to content

Conversation

@sd2k
Copy link
Contributor

@sd2k sd2k commented Jul 31, 2025

Similar to #181 but for node and npm. This has to be manually enabled
because we don't know the user's package manager.

@sd2k sd2k requested review from a team as code owners July 31, 2025 16:20
@sd2k sd2k requested review from oshirohugo, wbrowne and xnyo and removed request for a team July 31, 2025 16:20
Base automatically changed from fix-go-setup-caching-defaults to main July 31, 2025 18:14
Copy link
Collaborator

@xnyo xnyo left a comment

Choose a reason for hiding this comment

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

Great work! 🙌 I think this looks good and can be merged, but would love an opinion from @jackw, as he was looking into adding a shared action to detect the package manager being used in a plugin's repo.

Once that's introduced, we can make the two arguments optional and, when they're not provided, the package manager will be auto-detected. However we probably still need an input to disable caching if necessary.

I think we can merge this for now, but we may have to change/deprecate some inputs in the future. @jackw wdyt? maybe we should wait until we can auto-detect the package manager?

@xnyo xnyo requested a review from jackw August 1, 2025 08:19
@tolzhabayev
Copy link
Contributor

fixes #194

uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
with:
node-version: "${{ inputs.node-version }}"
cache: "${{ inputs.node-cache }}"
Copy link
Contributor

@tolzhabayev tolzhabayev Aug 4, 2025

Choose a reason for hiding this comment

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

do we know what happens when those inputs are empty strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Empty string is the default so it should work fine.

Comment on lines 37 to 40
node-cache:
description: Used to specify a package manager for caching in the default directory. Supported values npm, yarn, pnpm (https://github.com/actions/setup-node#caching-global-packages-data)
type: string
required: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed we already have a package-manager input so we could just use that...

@sd2k sd2k force-pushed the add-optional-node-caching branch 2 times, most recently from 817e168 to 2b40c64 Compare August 12, 2025 14:13
@sd2k
Copy link
Contributor Author

sd2k commented Aug 12, 2025

I've updated this to remove the node-cache input, which expected the package manager name, and use the existing package-manager input instead. Now the only new thing in the external ci/cd.yml files is the optional node-cache-dependency-path input which should take package.lock, yarn.lock etc.

I imagine we use @jackw's PR to automatically populate a sensible default here but the underlying setup-node action allows more customisation (e.g. for multiple package.lock files) so I don't think it's crazy to keep this option exposed - WDYT?

@sd2k sd2k force-pushed the add-optional-node-caching branch from 2b40c64 to 302e5ea Compare August 12, 2025 15:20
sd2k added 2 commits August 12, 2025 16:39
Similar to #181 but for node and npm. This has to be manually enabled
because we don't know the user's package manager.
@sd2k sd2k force-pushed the add-optional-node-caching branch from 302e5ea to 4a60108 Compare August 12, 2025 15:39
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