Skip to content

Commit

Permalink
Always run yarn install
Browse files Browse the repository at this point in the history
  • Loading branch information
@Shinigami92
Shinigami92 committed Jul 27, 2021
1 parent 4376790 commit 1dfbf99
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 4 deletions.
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
View file
Expand Up @@ -58,7 +58,7 @@ jobs:
yarn node -p process.versions
- name: Install dependencies
if: steps.yarn-cache.outputs.cache-hit != 'true'
# if: steps.yarn-cache.outputs.cache-hit != 'true'
run: yarn install --immutable

This comment has been minimized.

Sign in to view

Copy link
@fisker

fisker Jul 28, 2021

Member

https://github.com/actions/setup-node#caching-packages-dependencies

This comment has been minimized.

Sign in to view

Copy link
@Shinigami92

Shinigami92 Jul 28, 2021

Author Member

Coooool 😎

This comment has been minimized.

Sign in to view

Copy link
@Shinigami92

Shinigami92 Jul 28, 2021

Author Member

But I also added the node version into my cache key 🤔
And this variant will not do that

https://github.com/actions/setup-node/blob/aa759c6c94d3800c55b8601f21ba4b2371704cb7/src/cache-restore.ts#L19-L28

So I'm afraid of some conflicts 🤔
Do you assume that it should work anyway?

Currently my key is named: node-${{ matrix.node_version }}-os-${{ matrix.os }}-yarn-${{ hashFiles('**/yarn.lock') }}

This comment has been minimized.

Sign in to view

Copy link
@fisker

fisker Jul 28, 2021

Member

You mean skip install? I think you need run install anyway. But cache makes installation faster.

This comment has been minimized.

Sign in to view

Copy link
@fisker

fisker Jul 28, 2021

Member

Why node version matters?

This comment has been minimized.

Sign in to view

Copy link
@Shinigami92

Shinigami92 Jul 28, 2021

Author Member

You mean skip install?

No

Why node version matters?

I just assume it, but not sure at all
But there is e.g. a difference between <= node v14 and >= v15 in npm v6 vs v7
They have e.g. different lock versions
And therefore I just wanted to get sure that each environment just are encapsulated from each other

This comment has been minimized.

Sign in to view

Copy link
@fisker

fisker Jul 28, 2021

Member

I don't know, but setup-node action is very popular, I'm sure they'll fix if people found problem.

This comment has been minimized.

Sign in to view

Copy link
@Shinigami92

Shinigami92 Jul 28, 2021

Author Member

actions/setup-node#304 (comment)

This comment has been minimized.

Sign in to view

Copy link
@fisker

fisker Jul 28, 2021

Member

Yap, saw it.

This comment has been minimized.

Sign in to view

Copy link
@Shinigami92

Shinigami92 Jul 28, 2021

Author Member

It's really up to date
Let's wait what they think about it and maybe I or someone else could contribute and add an optional toggle to enable the inclusion of node version onto cache key

I understand their discussion about pnpm, that makes sense
But in yarn or npm it could be a bit different 🤔

This comment has been minimized.

Sign in to view

Copy link
@fisker

fisker Jul 28, 2021

Member

I thought it caches node_modules too before, I think it' safe.

This comment has been minimized.

Sign in to view

Copy link
@Shinigami92

Shinigami92 Jul 28, 2021

Author Member

I don't use pnp due to issues with vuepress
Therefore I have a node_modules and I observed (and tested) with and without cached node_modules beside .yarn/cache and with node_modules cached the link step is MUCH faster

This comment has been minimized.

Sign in to view

Copy link
@fisker

fisker Jul 28, 2021

Member

FYI: The Prettier core codebase enabled it prettier/prettier#11243, we didn't cache before , because we don't really care that much, installation only take seconds without cache, we run tests about 1 hour...

This comment has been minimized.

Sign in to view

Copy link
@fisker

fisker Jul 28, 2021 •
edited

Member

My personal projects don't use it ,

1, They usually fast enough.
2, I don't commit lock file.

This comment has been minimized.

Sign in to view

Copy link
@Shinigami92

Shinigami92 Jul 28, 2021

Author Member

Yeah sure, cache is not that much needed.
But it relieve servers and also makes it just a bit faster (from around >20s to <10s) 🤷

I like lock files due to always reproducible builds
I encountered issues in the past (in other projects) where bugfix versions of sub dependencies just broke everything 💥
And therefore I'm happy about lock files 😄

This comment has been minimized.

Sign in to view

Copy link
@fisker

fisker Jul 28, 2021

Member

IMO, lock file are very useful for packages that user won't install directly, eg devDependencies and packages will included by build script. But harmful for unpinned dependencies that user may install different versions, because tests may not running on latest version.

This comment has been minimized.

Sign in to view

Copy link
@Shinigami92

Shinigami92 Jul 28, 2021

Author Member

See actions/setup-node#304 (comment)

So I will stay on actions/cache for now

- name: Build
Expand Down Expand Up @@ -101,7 +101,7 @@ jobs:
node-14-os-ubuntu-latest-yarn-
- name: Prepare
if: steps.yarn-cache.outputs.cache-hit != 'true'
# if: steps.yarn-cache.outputs.cache-hit != 'true'
run: yarn install --immutable

- name: Lint
Expand Down Expand Up @@ -134,7 +134,7 @@ jobs:
node-14-os-ubuntu-latest-yarn-
- name: Prepare
if: steps.yarn-cache.outputs.cache-hit != 'true'
# if: steps.yarn-cache.outputs.cache-hit != 'true'
run: yarn install --immutable

- name: Audit production
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/docs.yml
View file
Expand Up @@ -35,7 +35,7 @@ jobs:
node-14-os-ubuntu-latest-yarn-
- name: Install dependencies
if: steps.yarn-cache.outputs.cache-hit != 'true'
# if: steps.yarn-cache.outputs.cache-hit != 'true'
run: yarn install --immutable

- name: Build VuePress site
Expand Down

0 comments on commit 1dfbf99

Please sign in to comment.