Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: move files to build cache after files are uploaded instead of copying them before uploads #753

Merged
merged 14 commits into from
Jan 20, 2025
+4,397 −2,682
Merged

fix: move files to build cache after files are uploaded instead of copying them before uploads #753

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1844993
fix: move files to cache after upload instead of copying them before
pieh Jan 16, 2025
0deac07
chore: edit demo site a bit to ensure caching changes continue to work
pieh Jan 16, 2025
d1154ad
chore: add heavy files for testing purposes
pieh Jan 16, 2025
9fbce46
fix: skip storing build cache when cli is used
pieh Jan 16, 2025
083fc2f
revert: remove debug changes
pieh Jan 16, 2025
cb01d0c
chore: drop debug console logs
pieh Jan 16, 2025
a82a6a5
fix: also move when restoring cache
pieh Jan 16, 2025
482a491
test: use older node (for gatsby v3)
pieh Jan 16, 2025
a60b5d7
fix: restoreCache types
pieh Jan 16, 2025
30ecbf5
fix: ensure all build step hooks have typed arguments
pieh Jan 16, 2025
4bd1abb
test: relax content-type check a bit
pieh Jan 16, 2025
7f6b7d7
test: pin netlify-cli
pieh Jan 17, 2025
1f942aa
test: use latest v5, log output on failure
pieh Jan 17, 2025
16bed3d
chore: upgrade @netlify/build
pieh Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jobs:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
node-version: '*'
node-version: '18'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- name: Global Node packages cache
uses: actions/cache@v3
with:
Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- run: npm ci
- run: cd plugin && npm ci && npm run build
- run: npm test
Expand All @@ -56,7 +56,7 @@ jobs:
key:
macOS-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
- run: npm ci
- run: cd plugin && npm ci && npm run build
- run: npm test
33 changes: 29 additions & 4 deletions plugin/src/helpers/cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,29 @@
import path from 'path'
import process from 'process'

import type { NetlifyPluginOptions } from '@netlify/build'

import { getGatsbyRoot } from './config'

function getCacheDirs(publish) {
return [publish, normalizedCacheDir(publish)]
}

export async function saveCache({ publish, utils }): Promise<void> {
export async function saveCache({
publish,
utils,
}: {
publish: string
utils: NetlifyPluginOptions['utils']
}): Promise<void> {
if (process.env.NETLIFY_LOCAL) {
return
}

const cacheDirs = getCacheDirs(publish)

if (await utils.cache.save(cacheDirs)) {
// @ts-expect-error - `move` is not in the types, but it is passed through to @netlify/cache-utils that support this option
mrstork marked this conversation as resolved.
Show resolved Hide resolved
if (await utils.cache.save(cacheDirs, { move: true })) {
utils.status.show({
title: 'Essential Gatsby Build Plugin ran successfully',
summary: 'Stored the Gatsby cache to speed up future builds. 🔥',
Expand All @@ -19,10 +33,21 @@ export async function saveCache({ publish, utils }): Promise<void> {
}
}

export async function restoreCache({ publish, utils }): Promise<void> {
export async function restoreCache({
publish,
utils,
}: {
publish: string
utils: NetlifyPluginOptions['utils']
}): Promise<void> {
if (process.env.NETLIFY_LOCAL) {
return
}

const cacheDirs = getCacheDirs(publish)

if (await utils.cache.restore(cacheDirs)) {
// @ts-expect-error - `move` is not in the types, but it is passed through to @netlify/cache-utils that support this option
if (await utils.cache.restore(cacheDirs, { move: true })) {
console.log('Found a Gatsby cache. We’re about to go FAST. ⚡️')
} else {
console.log('No Gatsby cache found. Building fresh.')
Expand Down
14 changes: 8 additions & 6 deletions plugin/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export async function onPreBuild({
constants,
utils,
netlifyConfig,
}): Promise<void> {
}: NetlifyPluginOptions): Promise<void> {
const { PUBLISH_DIR } = constants
// Print a helpful message if the publish dir is misconfigured
if (!PUBLISH_DIR || process.cwd() === path.resolve(PUBLISH_DIR)) {
Expand Down Expand Up @@ -115,14 +115,11 @@ The plugin no longer uses this and it should be deleted to avoid conflicts.\n`)

export async function onPostBuild({
constants: { PUBLISH_DIR, FUNCTIONS_DIST },
utils,
}): Promise<void> {
}: NetlifyPluginOptions): Promise<void> {
if (shouldSkip(PUBLISH_DIR)) {
return
}

await saveCache({ publish: PUBLISH_DIR, utils })
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@pieh pieh Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


const cacheDir = normalizedCacheDir(PUBLISH_DIR)

const neededFunctions = await getNeededFunctions(cacheDir)
Expand All @@ -132,11 +129,16 @@ export async function onPostBuild({
}
}

export async function onSuccess({ constants: { PUBLISH_DIR } }) {
export async function onSuccess({
constants: { PUBLISH_DIR },
utils,
}: NetlifyPluginOptions) {
if (shouldSkip(PUBLISH_DIR)) {
return
}

await saveCache({ publish: PUBLISH_DIR, utils })

// Pre-warm the lambdas as downloading the datastore file can take a while
if (shouldSkipBundlingDatastore()) {
const FETCH_TIMEOUT = 5000
Expand Down
6 changes: 3 additions & 3 deletions plugin/test/fixtures/v4/functions-without-gatsby-plugin/e2e-tests/test-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, is this just a preference or is there a hidden reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})
test(`returns json correctly via send`, async () => {
const res = await fetchTwice(`${host}/api/i-am-json-too`)
Expand All @@ -124,14 +124,14 @@ 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/)
})
test(`returns boolean correctly via send`, async () => {
const res = await fetchTwice(`${host}/api/i-am-false`)
const result = await res.json()

expect(result).toEqual(false)
expect(res.headers.get('content-type')).toEqual('application/json')
expect(res.headers.get('content-type')).toMatch(/^application\/json/)
})
test(`returns status correctly via send`, async () => {
const res = await fetchTwice(`${host}/api/i-am-status`)
Expand Down
6 changes: 3 additions & 3 deletions plugin/test/fixtures/v5/functions-without-gatsby-plugin/e2e-tests/test-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/)
})
test(`returns json correctly via send`, async () => {
const res = await fetchTwice(`${host}/api/i-am-json-too`)
Expand All @@ -124,14 +124,14 @@ 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/)
})
test(`returns boolean correctly via send`, async () => {
const res = await fetchTwice(`${host}/api/i-am-false`)
const result = await res.json()

expect(result).toEqual(false)
expect(res.headers.get('content-type')).toEqual('application/json')
expect(res.headers.get('content-type')).toMatch(/^application\/json/)
})
test(`returns status correctly via send`, async () => {
const res = await fetchTwice(`${host}/api/i-am-status`)
Expand Down
4 changes: 4 additions & 0 deletions plugin/test/fixtures/v5/with-adapters/e2e-tests/build.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ describe('A site using gatsby version with adapters', () => {
// in CI warnings are outputted to stderr (yikes)
const fullOutput = stdout + stderr

if (!success) {
console.error(fullOutput)
}

expect(success).toBeTruthy()

expect(fullOutput).toContain(
Expand Down
6 changes: 3 additions & 3 deletions plugin/test/fixtures/v5/with-adapters/e2e-tests/test-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,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/)
})
test(`returns json correctly via send`, async () => {
const res = await fetchTwice(`${host}/api/i-am-json-too`)
Expand All @@ -126,14 +126,14 @@ 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/)
})
test(`returns boolean correctly via send`, async () => {
const res = await fetchTwice(`${host}/api/i-am-false`)
const result = await res.json()

expect(result).toEqual(false)
expect(res.headers.get('content-type')).toEqual('application/json')
expect(res.headers.get('content-type')).toMatch(/^application\/json/)
})
test(`returns status correctly via send`, async () => {
const res = await fetchTwice(`${host}/api/i-am-status`)
Expand Down
2 changes: 1 addition & 1 deletion plugin/test/fixtures/v5/with-adapters/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"test:jest": "jest functions.test.js"
},
"dependencies": {
"gatsby": "5.12.5",
"gatsby": "5.14.1",
"gatsby-plugin-netlify": "5.1.0",
"react": "^18.2.0",
"react-dom": "^18.2.0"
Expand Down
Loading