[No QA] Cache node modules in GitHub Actions #20436
Conversation
melvin-bot
bot
requested review from
amyevans and
rushatgabhane
and removed request for
a team
|
@rushatgabhane @amyevans One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Opting to skip C+ review for this PR |
| key: ${{ runner.os }}-node-modules-${{ hashFiles('package-lock.json') }} | ||
|
|
||
| - name: Install node packages | ||
| if: github.run_attempt != '1' || steps.cache-node-modules.outputs.cache-hit != 'true' |
There was a problem hiding this comment.
Note: I tried using fromJSON here, but it doesn't work because steps.cache-node-modules.outputs.cache-hit is either true or null, not true or false as the documentation suggests.
There was a problem hiding this comment.
👍 I see this matches the example in the docs here
| uses: actions/cache@v3 | ||
| with: | ||
| path: node_modules | ||
| key: ${{ runner.os }}-node-modules-${{ hashFiles('package-lock.json') }} |
There was a problem hiding this comment.
Note: no partial matches are allowed, because npm ci will delete node_modules if it's present. It's either a full match and npm ci is skipped entirely, or it's a cache miss and we run npm ci. We do not do npm i because it can change package-lock.json and is considered unsafe for CI environments
| key: ${{ runner.os }}-node-modules-${{ hashFiles('package-lock.json') }} | ||
|
|
||
| - name: Install node packages | ||
| if: github.run_attempt != '1' || steps.cache-node-modules.outputs.cache-hit != 'true' |
There was a problem hiding this comment.
👍 I see this matches the example in the docs here
Reviewer Checklist
Screenshots/VideosWebN/A Mobile Web - ChromeN/A Mobile Web - SafariN/A DesktopN/A iOSN/A AndroidN/A |
|
Reading the docs: https://github.com/actions/setup-node#caching-global-packages-data Makes me think it should be working by default, why is it not? We could maybe cache even more using |
|
Yeah, the docs are confusing for sure. They say this:
But then soon after says this:
So I dug into it and discovered what they mean by |
|
Also, fwiw |
|
Ok this is great to confirm we need it, just a few more questions:
|
|
Some more info on what For example, if you have a dependency you're installing, it may be stored in the npm cache such that it doesn't have to be downloaded over the internet. The setup-node cache is basically the difference between running: # With caching
rm -rf node_modules
npm iand # Without caching
rm -rf node_modules
npm cache clean –force
npm iYou can run those both locally and you'll see why the setup-node cache is helpful 🙂 But it still doesn't cache our actual |
Where do you see that? Here's a workflow run I triggered about 20 minutes ago: https://github.com/Andrew-Test-Org/Public-Test-Repo/actions/runs/5213976292 And without changing the package-lock.json, I just triggered another: https://github.com/Andrew-Test-Org/Public-Test-Repo/actions/runs/5214138966/jobs/9410020020 and you can see that the cache was restores and we did not run |
Two PRs can share the cache of mains node_modules unless they updated
The only time the |
Good question. I can look into this |
|
Added caching for desktop node_modules. Also made it so that we run |
|
CPing to test the cocoapods caching change |
|
|
|
Some hiccups here, but was able to fix them in #20488 and we seem to be good-to-go here. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.27-0 🚀
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.27-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.27-7 🚀
|
Details
This PR caches node_modules in our GitHub Actions CI according to the following logic:
package-lock.jsonhasn't changed, and a cache is restored, skip installingnode_modulesbecause it's not necessary.package-lock.jsonhas changed, or a cache is not restored, runnpm ciFixed Issues
$ #20433
Tests
This workflow was tested in Public-Test-Repo:
cache miss: https://github.com/Andrew-Test-Org/Public-Test-Repo/actions/runs/5206794080/jobs/9393712554
cache hit (first run): https://github.com/Andrew-Test-Org/Public-Test-Repo/actions/runs/5206799767/jobs/9393722481
caching skipped (second run): https://github.com/Andrew-Test-Org/Public-Test-Repo/actions/runs/5206799767/jobs/9393730879
package-lock.json updated, cache miss: https://github.com/Andrew-Test-Org/Public-Test-Repo/actions/runs/5206813491/jobs/9393747848
Verify that no errors appear in the JS console
Offline tests
n/a
QA Steps
None.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android