jelhan / create-github-actions-setup-for-ember-addon

Setup GitHub Actions for an Ember Addon
MIT License
19 stars 2 forks source link

Simplify cache setup in created CI workflow #13

Closed jelhan closed 3 years ago

jelhan commented 3 years ago

Created CI workflow currently uses three steps to setup the cache of node_modules and global yarn or npm cache: https://github.com/jelhan/create-github-actions-setup-for-ember-addon/blob/e518896b9bed309ae419736f4e8f57d76cfa0c69/templates/.github/workflows/ci.yml#L20-L53

@ijlee2 pointed out in a review that it can be simplified to two steps with less code. It will also allow to simplify the if statement used in dependency installation: https://github.com/jelhan/ember-style-modifier/pull/32#discussion_r532058330

simonihmig commented 3 years ago

@jelhan first of all, nice work here! 😍

I also noticed the step of basically installing dependencies is quite verbose here. So just to bring another possible solution to the table: we are using an action (for all or private and public repos, including ember-bootstrap btw) that handles caching transparently: https://github.com/bahmutov/npm-install. Using that, there is much less to worry about. And it also supports both npm and yarn ootb.

What it does not do however AFAIK is that it caches both the npm/yarn cache as well as node_modules itself (it does only the former). But what I am really not sure about is how useful caching both is. The former will already prevent fetching from the network I believe. And the examples given by @actions/cache also show only the former version of caching: https://github.com/actions/cache/blob/main/examples.md#node---yarn 🤷‍♂️

jelhan commented 3 years ago

@simonihmig Thanks a lot for stepping in as well and sharing your experience! Very much appreciated.

What it does not do however AFAIK is that it caches both the npm/yarn cache as well as node_modules itself (it does only the former). But what I am really not sure about is how useful caching both is. The former will already prevent fetching from the network I believe.

Actually I also thought about that one. But I was wondering if we can skip caching npm/yarn cache if we cache node_modules.

yarn install should take care of updating a prepopulated node_modules if out of sync. I hope NPM is able to do the same this days.

Running yarn install takes a long time even if all packages are in local cache. I just benchmarked with a newly created addon. I run yarn install --offline in different scenarios:

All of this benchmarked on my personal laptop. May see different timings on GitHub Actions.

ijlee2 commented 3 years ago

@jelhan @simonihmig It's quite possible that we don't need to cache both npm/yarn cache and node_modules. There's always a possibility that I might have misunderstood how to cache things back in March.

Regarding caching for ember-try(https://github.com/jelhan/ember-style-modifier/pull/32#discussion_r532060378), I'm not sure either if we need to include the scenario name in the cache key for uniqueness. It's possible that we don't need to.

kategengler commented 3 years ago

A few comments on caching: