Add JS/CSS coding standard workflow #14
Conversation
|
|
||
| # Adding Node.js setup to run JS and CSS linters | ||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v2 |
There was a problem hiding this comment.
Any reason why you're using @v2 instead of @v3 which is the latest?
| ${{ runner.os }}-node- | ||
|
|
||
| - name: Install NPM dependencies | ||
| run: npm install |
There was a problem hiding this comment.
I think npm ci is more appropriate in this context: https://stackoverflow.com/a/53325242
| run: echo "::set-output name=dir::$(npm config get cache)" | ||
|
|
||
| - name: Cache npm dependencies | ||
| uses: actions/cache@v2 |
There was a problem hiding this comment.
Similarly, any reason why locking into @v2 instead of the latest @v3?
| uses: actions/cache@v2 | ||
| with: | ||
| path: | | ||
| **/node_modules |
There was a problem hiding this comment.
Where did you get the idea to cache the node_modules specifically? All tutorials I can find on this just use the outputs.dir folder from the line below 🤔
There was a problem hiding this comment.
Well, that was my idea, not sure if this is needed, I can remove it.
There was a problem hiding this comment.
I'd remove it based on this (taken from these docs):
Note It is not recommended to cache node_modules, as it can break across Node versions and won't work with npm ci
There was a problem hiding this comment.
I'm wondering if it wouldn't be better to simply use the cache directive in actions/setup-node directly: https://github.com/marketplace/actions/setup-node-js-environment#caching-global-packages-data
It's supposed to use actions/cache under the hood and is guaranteed to work and to simplify our entire action 🤔
| - name: Setup Node.js | ||
| uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: '18.15' # project version |
There was a problem hiding this comment.
Do you think we could automate this number by replacing node-version with node-version-file: 'package.json'? https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#node-version-file
Changes proposed in this Pull Request
lint:scriptsandint:stylesscriptsTesting instructions