-
Notifications
You must be signed in to change notification settings - Fork 364
feat: add support for pnpm (install and cache deps) #586
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
Conversation
} else if (usePnpm()) { | ||
o.inputPath = NPM_CACHE_FOLDER | ||
} else { | ||
o.inputPath = NPM_CACHE_FOLDER | ||
} |
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.
Can be redundant with the else
branch, but I prefer to be explicit here.
Friendly ping @admah 🙏 |
@Kocal thank you for submitting this. I will be taking a look at it tomorrow. |
Thanks a lot :) |
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.
Thanks for adding this. Looks great!
🎉 This PR is included in version 4.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Thanks you for merging :) |
Thanks for this PR! Just a quick heads up that this doesn't work in In general I'm somewhat surprised that and have a feeling that this github action is trying to do to much in one. Installing npm/pnpm/yarn packages and caching them is a well-solved problem. In case of pnpm for instance caching the store is actually preferable (instead of caching the node modules directory). So I'm wondering if it would maybe be better to simply remove all that stuff from this github action altogether and leave that to the use and only be concerned with actually running cypress. |
Yeah I also agree with you. I think all the install/caching dependencies things were here because it was painful/verbose-for-nothing, at the beginning of GitHub Actions, exemple: - name: Setup node
uses: actions/setup-node@v1
with:
node-version: 10
- name: Get npm cache directory
id: npm-cache-dir
run: |
echo "::set-output name=dir::$(npm config get cache)"
- uses: actions/cache@v3
id: npm-cache # use this to check for `cache-hit` ==> if: steps.npm-cache.outputs.cache-hit != 'true'
with:
path: ${{ steps.npm-cache-dir.outputs.dir }}
key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }}
restore-keys: |
${{ runner.os }}-node-
- run: npm install But since https://github.com/actions/setup-node/releases/tag/v2.2.0, you can simply use the - name: Setup node
uses: actions/setup-node@v3
with:
node-version: '14'
cache: 'npm'
- run: npm install
IMO this action should only:
and that's all. But that's another issue I guess. |
Close #145