Add cache to node workflows

[oscard0m]

Description

Follow up on this: https://github.com/prettier/eslint-config-prettier/pull/195

[oscard0m]

Sorry for the delay on this @lydell , just trying to summarise my knowledge:

Yes, feel free to experiment! Would be interesting to see if there’s any time difference,

I just added these changes in this new PR. My plan is to compare executions times of this PR CheckRuns with this one: https://github.com/prettier/eslint-config-prettier/pull/209. I guess we can take some initial conclusions.

and if npm ci --offline succeeds when there is a cache (to verify that the cache helps against npm outage).

I'm not use if I'm understanding, do you want your CI to succeed if there is an outage of npm? Would not be safer to wait until npm is stable?

---

Status: IMO the current setup works just fine, and I don’t really understand the new way of caching. Do you still feel like experimenting with this? Otherwise I think we’re going to close this one (unless you convince me that the new caching approach is better.)

My main goal when opening this PR was to simplify the yaml file of repository's workflows. Adding this cache option gives the option to remove the actions/cache step, which simplifies the file a little bit in my opinion. Actually, that's one of the goals setup-node team was looking for.

On the other hand, after opening the PR I noticed you were using actions/cache in a non-usual way, seems like these workflows are caching node_modules folder instead of ~/.npm (something you pointed too).

The main lowlight of this, is that these workflows are not getting the benefit from npm's cache when running it agains different node versions and operating systems (specially in test.yml). You can read how actions/cache is expected to be used here

Conclusion

I see 2 opportunities here:

• A performance improvement, specially in test.yml but be ready in case check.yml's matrix grows.
• Simpler workflows with one step less
• Getting benefits from future iterations of cache option from setup-node
• Not sure if there's a performance benefit, but removing the oeverhed of actions/cache could be an opportunity of a performance improvement too.

Let's see the numbers between https://github.com/prettier/eslint-config-prettier/actions/runs/1349639381 and https://github.com/prettier/eslint-config-prettier/actions/runs/1349647399

[oscard0m]

Comparison of Install dependencies: npm ci step:

	this PR	test-ci PR
ubuntu-latest, 10.x	8s	9s
ubuntu-latest, 12.x	11s	10s
ubuntu-latest, 14.x	10s	9s
ubuntu-latest, 15.x	20s	23s
windows-latest, 10.x	24s	22s
windows-latest, 12.x	20s	23s
windows-latest, 14.x	25s	23s
windows-latest, 15.x	33s	34s
macOS-latest, 10.x	13s	13s
macOS-latest, 12.x	13s	23s
macOS-latest, 14.x	22s	21s
macOS-latest, 15.x	24s	28s

Here's setup-node's ADR where there's some expected time improvements per platform: https://github.com/actions/setup-node/issues/271#issuecomment-871408337

[lydell]

Cool! Are those numbers for a run with or without cache? It would be interesting to see both!

[oscard0m]

Cool! Are those numbers for a run with or without cache? It would be interesting to see both!

In my understanding, test.yml is using cache already. Per each combination of ${os-node_version}, the same dependencies are installed do the cache enters in action supposedly.

[lydell]

I mean, the thing with caches is that sometimes you get a cache hit and sometimes you get a cache miss. Since one PR is running with the new stuff, and one with the old I was worried the new one ran with a cold cache but the old one with a warm cache. That would be an unfair comparison.

[lydell]

I’ve done some testing in both branches now.

With a warm cache in both, this new approach seems to be ~5 seconds slower, because now we spend ~5 seconds running npm ci whereas the old solution skips running npm ci at all when the there’s a cache hit.

I like the config simplification, and those 5 seconds don’t really matter. But this is very interesting. The solution we already have is easy to understand – if we already have a node_modules folder there’s nothing to do. The new is more difficult to understand. It saves ~/.npm instead so we still need to run npm ci but it takes less time, and if everything works as intended no network calls should be made. And it always needs to re-run postinstall hooks? Luckily, I don’t think any dependencies in this repo has any postinstall hooks.

I need to think more about this / test more and make a decision, but things do look a bit like there was nothing really wrong with the old setup – it even seems to be faster! – so if it ain’t broke don’t fix it? 🤔

[oscard0m]

I’ve done some testing in both branches now.

With a warm cache in both, this new approach seems to be ~5 seconds slower, because now we spend ~5 seconds running npm ci whereas the old solution skips running npm ci at all when the there’s a cache hit.

I like the config simplification, and those 5 seconds don’t really matter. But this is very interesting. The solution we already have is easy to understand – if we already have a node_modules folder there’s nothing to do. The new is more difficult to understand. It saves ~/.npm instead so we still need to run npm ci but it takes less time, and if everything works as intended no network calls should be made. And it always needs to re-run postinstall hooks? Luckily, I don’t think any dependencies in this repo has any postinstall hooks.

I need to think more about this / test more and make a decision, but things do look a bit like there was nothing really wrong with the old setup – it even seems to be faster! – so if it ain’t broke don’t fix it? 🤔

Very cool study and analysis you did here @lydell, I guess the difference is in this skip step logic you are doing in the old version maybe? Should we bump at least to setup-node@v2 at least (I can open a PR for that if interested)? Maybe there are some improvements :)

Let me know if you do a deeper research in terms of performance differences and reasons behind

[oscard0m]

I’ve done some testing in both branches now. With a warm cache in both, this new approach seems to be ~5 seconds slower, because now we spend ~5 seconds running npm ci whereas the old solution skips running npm ci at all when the there’s a cache hit. I like the config simplification, and those 5 seconds don’t really matter. But this is very interesting. The solution we already have is easy to understand – if we already have a node_modules folder there’s nothing to do. The new is more difficult to understand. It saves ~/.npm instead so we still need to run npm ci but it takes less time, and if everything works as intended no network calls should be made. And it always needs to re-run postinstall hooks? Luckily, I don’t think any dependencies in this repo has any postinstall hooks. I need to think more about this / test more and make a decision, but things do look a bit like there was nothing really wrong with the old setup – it even seems to be faster! – so if it ain’t broke don’t fix it? 🤔

Very cool study and analysis you did here @lydell, I guess the difference is in this skip step logic you are doing in the old version maybe? Should we bump at least to setup-node@v2 at least (I can open a PR for that if interested)? Maybe there are some improvements :)

Let me know if you do a deeper research in terms of performance differences and reasons behind

Hey @lydell , just a friendly follow up on this. I would like to know if we should try something here (bumping to v2 maybe?) or just close this PR.

Thanks once again for all the knowledge and effort you put on this. I learnt A LOT!

---

EDIT: @lydell did you have time to read my last comment here? :)

[lydell]

It looks like this isn’t happening. Thanks for teaching me about the new cache stuff in setup-node though!