-
Notifications
You must be signed in to change notification settings - Fork 87
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.
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: make revalidateTags
no-op when list of tags is empty
#2727
Changes from 6 commits
d7b3458
04e5451
28339a3
d0a7f47
782a106
f112f29
4a02e34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { unstable_cache } from 'next/cache' | ||
|
||
export const dynamic = 'force-dynamic' | ||
|
||
const getData = unstable_cache( | ||
async () => { | ||
return { | ||
timestamp: Date.now(), | ||
} | ||
}, | ||
[], | ||
{ | ||
revalidate: 1, | ||
}, | ||
) | ||
|
||
export default async function Page() { | ||
const data = await getData() | ||
|
||
return <pre>{JSON.stringify(data, null, 2)}</pre> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ import { cp } from 'node:fs/promises' | |
import { createRequire } from 'node:module' | ||
import { join } from 'node:path' | ||
import { gunzipSync } from 'node:zlib' | ||
import { HttpResponse, http, passthrough } from 'msw' | ||
import { setupServer } from 'msw/node' | ||
import { gt, prerelease } from 'semver' | ||
import { v4 } from 'uuid' | ||
import { Mock, afterAll, beforeAll, beforeEach, describe, expect, test, vi } from 'vitest' | ||
|
@@ -22,6 +24,8 @@ import { | |
startMockBlobStore, | ||
} from '../utils/helpers.js' | ||
import { nextVersionSatisfies } from '../utils/next-version-helpers.mjs' | ||
import { purgeCache } from '@netlify/functions' | ||
import { afterEach } from 'node:test' | ||
|
||
const mockedCp = cp as Mock< | ||
Parameters<(typeof import('node:fs/promises'))['cp']>, | ||
|
@@ -36,9 +40,32 @@ vi.mock('node:fs/promises', async (importOriginal) => { | |
} | ||
}) | ||
|
||
let server: ReturnType<typeof setupServer> | ||
|
||
// Disable the verbose logging of the lambda-local runtime | ||
getLogger().level = 'alert' | ||
|
||
const purgeAPI = vi.fn() | ||
|
||
beforeAll(() => { | ||
server = setupServer( | ||
http.post('https://api.netlify.com/api/v1/purge', async ({ request }) => { | ||
purgeAPI(await request.json()) | ||
|
||
return HttpResponse.json({ | ||
ok: true, | ||
}) | ||
}), | ||
http.all(/.*/, () => passthrough()), | ||
) | ||
server.listen() | ||
}) | ||
|
||
afterAll(() => { | ||
// Disable API mocking after the tests are done. | ||
server.close() | ||
}) | ||
|
||
beforeEach<FixtureTestContext>(async (ctx) => { | ||
// set for each test a new deployID and siteID | ||
ctx.deployID = generateRandomObjectID() | ||
|
@@ -48,9 +75,15 @@ beforeEach<FixtureTestContext>(async (ctx) => { | |
// hide debug logs in tests | ||
vi.spyOn(console, 'debug').mockImplementation(() => {}) | ||
|
||
purgeAPI.mockClear() | ||
|
||
await startMockBlobStore(ctx) | ||
}) | ||
|
||
afterEach(() => { | ||
vi.unstubAllEnvs() | ||
}) | ||
|
||
test<FixtureTestContext>('Test that the simple next app is working', async (ctx) => { | ||
await createFixture('simple', ctx) | ||
await runPlugin(ctx) | ||
|
@@ -210,6 +243,39 @@ test<FixtureTestContext>('cacheable route handler is cached on cdn (revalidate=f | |
) | ||
}) | ||
|
||
test<FixtureTestContext>('purge API is not used when unstable_cache cache entry gets stale', async (ctx) => { | ||
await createFixture('simple', ctx) | ||
await runPlugin(ctx) | ||
|
||
// set the NETLIFY_PURGE_API_TOKEN to get pass token check and allow fetch call to be made | ||
vi.stubEnv('NETLIFY_PURGE_API_TOKEN', 'mock') | ||
|
||
const page1 = await invokeFunction(ctx, { | ||
url: '/unstable_cache', | ||
}) | ||
const data1 = load(page1.body)('pre').text() | ||
|
||
// allow for cache entry to get stale | ||
await new Promise((res) => setTimeout(res, 2000)) | ||
Comment on lines
+266
to
+267
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is mocking and advancing the clock here problematic? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we use above in plenty of other places that check behaviors related to stale cache items so I just the same method here I'm not sure if attempting to mock time would work - conceptually it should of course, but not sure it's worth doing that as most time is spent on setting up fixtures rather than running the test itself anyway (at least in this case) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll go ahead and keep this as is in effort to get this fix out and would say moving to mocking time instead of using ~ |
||
|
||
const page2 = await invokeFunction(ctx, { | ||
url: '/unstable_cache', | ||
}) | ||
const data2 = load(page2.body)('pre').text() | ||
|
||
const page3 = await invokeFunction(ctx, { | ||
url: '/unstable_cache', | ||
}) | ||
const data3 = load(page3.body)('pre').text() | ||
|
||
expect(purgeAPI, 'Purge API should not be hit').toHaveBeenCalledTimes(0) | ||
expect( | ||
data2, | ||
'Should use stale cache entry for current request and invalidate it in background', | ||
).toBe(data1) | ||
expect(data3, 'Should use updated cache entry').not.toBe(data2) | ||
}) | ||
|
||
test<FixtureTestContext>('cacheable route handler is cached on cdn (revalidate=15)', async (ctx) => { | ||
await createFixture('simple', ctx) | ||
await runPlugin(ctx) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 shouldn't this be from
vitest
like all the others?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooh, yes, this was result of import being automatically added and me not noticing it was wrong one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4a02e34