fix: move files to build cache after files are uploaded instead of copying them before uploads #753
Conversation
✅ Deploy Preview for netlify-plugin-gatsby-demo-v5 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for netlify-plugin-gatsby-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
pieh
force-pushed
the
fix/optimize-build-cache
branch
from
ee8fc67 to
083fc2f
Compare
pieh
changed the title
| @@ -14,7 +14,7 @@ jobs: | |||
| - uses: actions/checkout@v3 | |||
| - uses: actions/setup-node@v3 | |||
| with: | |||
| node-version: '*' | |||
| node-version: '18' | |||
There was a problem hiding this comment.
gatsby v3 which is tested doesn't work on newer nodes - https://github.com/netlify/netlify-plugin-gatsby/actions/runs/12812982624/job/35725952139#step:9:950
pieh
force-pushed
the
fix/optimize-build-cache
branch
from
9a98978 to
4bd1abb
Compare
pieh
force-pushed
the
fix/optimize-build-cache
branch
from
57063ea to
7f6b7d7
Compare
| @@ -29,7 +29,7 @@ jobs: | |||
| key: | |||
| ubuntu-build-${{ env.cache-name }}-${{ | |||
| hashFiles('plugin/test/fixtures/**/package.json') }}-node-modules | |||
| - run: npm install -g netlify-cli | |||
| - run: npm install -g netlify-cli@18.0.0 | |||
There was a problem hiding this comment.
there seems to be regression in latest version of cli (18.0.1) that result in skipping rewrites to .netlify/functions/__api and hitting framework's dev server directly which trips up the tests - I'm pinning it for now to "fix" the tests
I suspect it's caused by netlify/cli#6994 and reported in https://netlify.slack.com/archives/C07686YAY13/p1737134106866159?thread_ts=1736521386.323879&cid=C07686YAY13
| @@ -115,7 +115,7 @@ exports.runTests = function runTests(env, host) { | |||
| expect(result).toEqual({ | |||
| amIJSON: true, | |||
| }) | |||
| expect(res.headers.get('content-type')).toEqual('application/json') | |||
| expect(res.headers.get('content-type')).toMatch(/^application\/json/) | |||
There was a problem hiding this comment.
Just out of curiosity, is this just a preference or is there a hidden reason for this change?
There was a problem hiding this comment.
I was seeing those assertion failures ( https://github.com/netlify/netlify-plugin-gatsby/actions/runs/12813658707/job/35728241714#step:9:1832 )
● Local › response formats › returns json correctly via send
expect(received).toEqual(expected) // deep equality
Expected: "application/json"
Received: "application/json; charset=utf-8"
125 | amIJSON: true,
126 | })
> 127 | expect(res.headers.get('content-type')).toEqual('application/json')
| ^
128 | })
129 | test(`returns boolean correctly via send`, async () => {
130 | const res = await fetchTwice(`${host}/api/i-am-false`)
at Object.<anonymous> (e2e-tests/test-helpers.js:127:49)
so I adjusted assertions to ensure application/json type but to not strongly check for additional directives
| if (shouldSkip(PUBLISH_DIR)) { | ||
| return | ||
| } | ||
|
|
||
| await saveCache({ publish: PUBLISH_DIR, utils }) |
There was a problem hiding this comment.
Now that we are moving the cache around, not just copying, if we only save the cache on successful builds are we essentially clearing the cache on failed build? Is it potentially worth keeping this save cache here?
There was a problem hiding this comment.
Thanks for catching this - I spent most time on this PR fixing tests (after not touching this repo for months) and this did slip. Overall I don't want to introduce changes to failed builds behavior, so I'm gonna investigate what is exact behavior now (wether we store or not cache on failed builds) and ensure this behavior is preserved
There was a problem hiding this comment.
Ok. onPostBuild is NOT executed on build failures - so moving this only to onSuccess (and not in onError or onEnd) is preserving current behavior (at least when it comes to scenarios when we do store build cache)
There might be separate thing here to maybe start storing cache on failed builds, but this is out of scope for this PR.
There was a problem hiding this comment.
Ahh, I think I missed what the most important bit here - when we restore cache - we now move the files out and not just copy them - so if build fails - the cache location no longer has original cache files which would possibly result in subsequent build losing any of previous caches. The part I'm not sure about is wether we do upload that cache on failed builds and will look into that now
There was a problem hiding this comment.
It seems like we don't actually store cache from cache location on failed builds either, so this is still preserving the behavior.
BuildBot build command stage would fail on build failure and this would result in subsequent stages not running anymore (save cache is one of last stages after build command)
pieh
force-pushed
the
fix/optimize-build-cache
branch
from
7d20e68 to
16bed3d
Compare
| @@ -21,7 +21,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@sindresorhus/slugify": "^1.1.2", | |||
| "gatsby": "next", | |||
| "gatsby": "5.11.0", | |||
There was a problem hiding this comment.
last version pre adapters that result in build plugin actually doing anything
Summary
We are currently copying
.cache(gatsby cache dir) andpublic(gatsby publish dir) to cache directory. This results in almost doubling disk space usage for sites that produce a lot and/or heavy static assets and seems unnecessary - this change should result in lowering disk usage, but also should speed up handling of caching due to moving files and not copying themRelevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal
Fixes https://linear.app/netlify/issue/FRB-1576/research-changing-netlify-plugin-gatsby-behavior-to-move-files-to
Standard checks:
🧪 Once merged, make sure to update the version if needed and that it was
published correctly.