Skip to content
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

Enable caching by default if packageManager field is defined in package.json #686

Open
trivikr opened this issue Jan 31, 2023 · 3 comments
Labels
feature request New feature or request to improve the current logic

Comments

@trivikr
Copy link
Contributor

trivikr commented Jan 31, 2023

Description:
The action allows caching global packages data as per documentation in https://github.com/actions/setup-node#caching-global-packages-data

It's an amazing feature and reduces action execution time, but it has to be enabled in configuration.
Caching is not enabled in default setup. Users may not discover caching feature by default, and miss on the faster executions times enabled by caching.

The feature request is to enable caching by default if packageManager field is defined.
This can be enabled in a major version bump, say v4, to not affect existing users.

Justification:
Just want all users to get benefit of faster executions times allowed by caching without explicitly opting in, if they have set up packageManager field in their package.json

They can always explicitly opt-out by setting cache to empty string.

Are you willing to submit a PR?
Yes

@trivikr trivikr added feature request New feature or request to improve the current logic needs triage labels Jan 31, 2023
@trivikr
Copy link
Contributor Author

trivikr commented Jan 31, 2023

I think the implementation can be as simple as

-    if (cache && isCacheFeatureAvailable()) {
+    const packageManagerFromManifest = getNameFromPackageManagerField();
+    if ((cache !== "" || packageManagerFromManifest) && isCacheFeatureAvailable()) {
       const cacheDependencyPath = core.getInput('cache-dependency-path');
-      await restoreCache(cache, cacheDependencyPath);
+      const packageManager = cache !== "" ? cache : packageManagerFromManifest;
+      await restoreCache(packageManager, cacheDependencyPath);
     }

where getNameFromPackageManagerField returns the package manager name from packageManager field. That is:

Other package managers can be added, once they are supported

@MaksimZhukov
Copy link
Contributor

Hello @trivikr ! Thank you for the suggested idea!
We will consider adding this feature and will let you know as soon as we have any decision.

@Airkro
Copy link

Airkro commented Sep 16, 2023

This is what I doing before this feature landed:

jobs:
  steps:
    - name: Checkout
      uses: actions/checkout@v3

    - name: Detect package manager
      shell: bash
      run: node -p "'PACKAGE_MANAGER='+require('./package.json').packageManager?.split?.('@')?.[0]" >> "$GITHUB_ENV"

    - name: Install pnpm
      run: corepack enable pnpm
      if: ${{ env.PACKAGE_MANAGER == 'pnpm' }}
      # install pnpm before setup-node

    - name: Setup node
      uses: actions/setup-node@v3
      with:
        node-version: lts
        cache: ${{ env.PACKAGE_MANAGER }}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request to improve the current logic
Projects
None yet
Development

No branches or pull requests

3 participants