-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(start): retry failed prerenders and fail the build on error #8171
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
base: main
Are you sure you want to change the base?
Changes from all commits
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,5 @@ | ||
| --- | ||
| '@tanstack/start-plugin-core': patch | ||
| --- | ||
|
|
||
| Fix prerendering so that `retryCount` actually retries a failed page, and a page that still fails with `failOnError` enabled now fails the build instead of exiting successfully. |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -86,6 +86,7 @@ export async function prerender({ | |||||||||
| const seen = new Set<string>() | ||||||||||
| const prerendered = new Set<string>() | ||||||||||
| const retriesByPath = new Map<string, number>() | ||||||||||
| const errors: Array<unknown> = [] | ||||||||||
| const concurrency = startConfig.prerender?.concurrency ?? os.cpus().length | ||||||||||
| logger.info(`Concurrency: ${concurrency}`) | ||||||||||
| const queue = new Queue({ concurrency }) | ||||||||||
|
|
@@ -106,14 +107,24 @@ export async function prerender({ | |||||||||
|
|
||||||||||
| await queue.start() | ||||||||||
|
|
||||||||||
| if (errors.length > 0) { | ||||||||||
| if (errors.length === 1) { | ||||||||||
| throw errors[0] | ||||||||||
| } | ||||||||||
| throw new AggregateError( | ||||||||||
| errors, | ||||||||||
| `Prerendering failed for ${errors.length} pages`, | ||||||||||
| ) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return Array.from(prerendered) | ||||||||||
|
|
||||||||||
| function addCrawlPageTask(page: Page) { | ||||||||||
| if (seen.has(page.path)) return | ||||||||||
|
Contributor
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. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Add braces to the Use braces around this Proposed fix- if (seen.has(page.path)) return
+ if (seen.has(page.path)) {
+ return
+ }As per coding guidelines, “Always use curly braces for 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||
|
|
||||||||||
| seen.add(page.path) | ||||||||||
|
|
||||||||||
| if (page.fromCrawl) { | ||||||||||
| if (page.fromCrawl && !startConfig.pages.includes(page)) { | ||||||||||
| startConfig.pages.push(page) | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -219,9 +230,10 @@ export async function prerender({ | |||||||||
| ) | ||||||||||
| await new Promise((resolve) => setTimeout(resolve, retryDelay)) | ||||||||||
| retriesByPath.set(page.path, retries + 1) | ||||||||||
| seen.delete(page.path) | ||||||||||
| addCrawlPageTask(page) | ||||||||||
| } else if (prerenderOptions.failOnError ?? true) { | ||||||||||
| throw error | ||||||||||
| errors.push(error) | ||||||||||
| } | ||||||||||
| } | ||||||||||
| }) | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| import { describe, expect, it, vi } from 'vitest' | ||
| import { prerender } from '../src/prerender' | ||
|
|
||
| vi.mock('../src/utils', async () => { | ||
| const actual = await vi.importActual<any>('../src/utils') | ||
| return { | ||
| ...actual, | ||
| createLogger: () => ({ info: () => {}, warn: () => {}, error: () => {} }), | ||
| } | ||
| }) | ||
|
|
||
| // Mock fs to prevent actual file system operations | ||
| vi.mock('node:fs', async () => { | ||
| const actual = await vi.importActual<any>('node:fs') | ||
| return { | ||
| ...actual, | ||
| promises: { | ||
| ...actual.promises, | ||
| mkdir: vi.fn().mockResolvedValue(undefined), | ||
| writeFile: vi.fn().mockResolvedValue(undefined), | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
| function okResponse() { | ||
| return new Response('<html></html>', { | ||
| status: 200, | ||
| headers: { 'content-type': 'text/html' }, | ||
| }) | ||
| } | ||
|
|
||
| function failResponse() { | ||
| return new Response('boom', { status: 500 }) | ||
| } | ||
|
|
||
| function makeStartConfig( | ||
| pagePath: string, | ||
| prerenderOverrides: Record<string, unknown>, | ||
| ) { | ||
| return { | ||
| prerender: { | ||
| enabled: true, | ||
| autoStaticPathsDiscovery: false, | ||
| concurrency: 1, | ||
| crawlLinks: false, | ||
| retryDelay: 0, | ||
| ...prerenderOverrides, | ||
| }, | ||
| pages: [{ path: pagePath }], | ||
| router: { basepath: '' }, | ||
| spa: { | ||
| enabled: false, | ||
| prerender: { | ||
| outputPath: '/_shell', | ||
| crawlLinks: false, | ||
| retryCount: 0, | ||
| enabled: true, | ||
| }, | ||
| }, | ||
| } as any | ||
|
Comment on lines
+36
to
+60
Contributor
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. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Remove the
As per coding guidelines, “Use TypeScript strict mode with extensive type safety.” 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| } | ||
|
|
||
| describe('prerender retry and failOnError', () => { | ||
| it('retries a failing page up to retryCount times until it succeeds', async () => { | ||
| const request = vi | ||
| .fn() | ||
| .mockResolvedValueOnce(failResponse()) | ||
| .mockResolvedValueOnce(failResponse()) | ||
| .mockResolvedValue(okResponse()) | ||
| const handler = { getClientOutputDirectory: () => '/client', request } | ||
| const startConfig = makeStartConfig('/flaky', { | ||
| retryCount: 2, | ||
| failOnError: true, | ||
| }) | ||
|
|
||
| await expect(prerender({ startConfig, handler })).resolves.not.toThrow() | ||
|
Contributor
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. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: printf '%s\n' '--- repository conventions ---'
find /tmp/coderabbit-repo-knowledge/tanstack-router-7628dab7 -type f -name '*.md' -maxdepth 3 -print
printf '%s\n' '--- target test ---'
cat -n packages/start-plugin-core/tests/prerender-retry.test.ts | sed -n '1,145p'
printf '%s\n' '--- prerender definitions and relevant callers ---'
rg -n -A18 -B8 '(^|[^[:alnum:]_])prerender[[:space:]]*[:=(]|function[[:space:]]+prerender|async[[:space:]]+prerender|prerender\\(' packages/start-plugin-core packages -g '*.{ts,tsx,js,jsx}'Repository: TanStack/router Length of output: 7087 🏁 Script executed: printf '%s\n' '--- applicable repository conventions ---'
cat /tmp/coderabbit-repo-knowledge/tanstack-router-7628dab7/conventions/repo-wide.md
printf '%s\n' '--- prerender source map ---'
ast-grep outline packages/start-plugin-core/src/prerender.ts
printf '%s\n' '--- prerender implementation ---'
cat -n packages/start-plugin-core/src/prerender.ts | sed -n '1,260p'Repository: TanStack/router Length of output: 11093 🌐 Web query:
💡 Result: The error stating that the "received value must be a function" occurs when using the toThrow matcher because Vitest (and Jest) needs to wrap the execution of your code in a controlled environment to catch the error [1][2]. If you call a function directly inside expect—for example, expect(myFunction).toThrow—the function executes immediately, throws the error before Vitest can intercept it, and causes the test to crash with an unhandled exception [2]. To fix this, you must wrap your code in an anonymous function (a thunk) so that Vitest can call it and catch the error internally [3][2]: // Correct approach for synchronous functions expect( => myFunction).toThrow; For asynchronous functions or promises, the approach is different: 1. Do not use toThrow directly with expect. 2. Use the.rejects helper instead, which is designed to wait for a promise to reject [1][4]. // Correct approach for asynchronous functions await expect(myAsyncFunction).rejects.toThrow; If you are using a newer version of Vitest that supports it, you can also pass a function that returns a promise to.rejects (or.resolves, depending on your use case) [5]: // Correct approach for async function wrapper await expect( => myAsyncFunction).rejects.toThrow; Citations:
Use a resolution matcher for successful calls.
🤖 Prompt for AI Agents |
||
| // 1 initial attempt + 2 retries, succeeding on the third | ||
| expect(request).toHaveBeenCalledTimes(3) | ||
| }) | ||
|
|
||
| it('fails the build when a page fails and failOnError is set', async () => { | ||
| const request = vi.fn().mockResolvedValue(failResponse()) | ||
| const handler = { getClientOutputDirectory: () => '/client', request } | ||
| const startConfig = makeStartConfig('/broken', { | ||
| retryCount: 0, | ||
| failOnError: true, | ||
| }) | ||
|
|
||
| await expect(prerender({ startConfig, handler })).rejects.toThrow( | ||
| /Failed to fetch/, | ||
| ) | ||
| expect(request).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('retries then fails the build when the page never recovers', async () => { | ||
| const request = vi.fn().mockResolvedValue(failResponse()) | ||
| const handler = { getClientOutputDirectory: () => '/client', request } | ||
| const startConfig = makeStartConfig('/broken', { | ||
| retryCount: 2, | ||
| failOnError: true, | ||
| }) | ||
|
|
||
| await expect(prerender({ startConfig, handler })).rejects.toThrow( | ||
| /Failed to fetch/, | ||
| ) | ||
| // 1 initial attempt + 2 retries before giving up | ||
| expect(request).toHaveBeenCalledTimes(3) | ||
| }) | ||
|
|
||
| it('does not fail the build when failOnError is disabled', async () => { | ||
| const request = vi.fn().mockResolvedValue(failResponse()) | ||
| const handler = { getClientOutputDirectory: () => '/client', request } | ||
| const startConfig = makeStartConfig('/broken', { | ||
| retryCount: 0, | ||
| failOnError: false, | ||
| }) | ||
|
|
||
| await expect(prerender({ startConfig, handler })).resolves.not.toThrow() | ||
| }) | ||
|
|
||
| it('records a retried crawled page only once', async () => { | ||
| let childAttempts = 0 | ||
| const request = vi.fn((path: string) => { | ||
| if (path.includes('child')) { | ||
| childAttempts++ | ||
| return Promise.resolve( | ||
| childAttempts === 1 ? failResponse() : okResponse(), | ||
| ) | ||
| } | ||
| return Promise.resolve( | ||
| new Response('<html><a href="/child">child</a></html>', { | ||
| status: 200, | ||
| headers: { 'content-type': 'text/html' }, | ||
| }), | ||
| ) | ||
| }) | ||
| const handler = { getClientOutputDirectory: () => '/client', request } | ||
| const startConfig = makeStartConfig('/', { | ||
| crawlLinks: true, | ||
| retryCount: 1, | ||
| failOnError: false, | ||
| }) | ||
|
|
||
| await prerender({ startConfig, handler }) | ||
|
|
||
| // The crawled page fails once and is retried, but must be recorded once. | ||
| const childEntries = startConfig.pages.filter( | ||
| (page: { path: string }) => page.path === '/child', | ||
| ) | ||
| expect(childEntries).toHaveLength(1) | ||
| }) | ||
|
|
||
| it('aggregates multiple page failures into an AggregateError', async () => { | ||
| const request = vi.fn().mockResolvedValue(failResponse()) | ||
| const handler = { getClientOutputDirectory: () => '/client', request } | ||
| const startConfig = makeStartConfig('/a', { | ||
| retryCount: 0, | ||
| failOnError: true, | ||
| }) | ||
| startConfig.pages = [{ path: '/a' }, { path: '/b' }] | ||
|
|
||
| let error: unknown | ||
| try { | ||
| await prerender({ startConfig, handler }) | ||
| } catch (e) { | ||
| error = e | ||
| } | ||
|
|
||
| expect(error).toBeInstanceOf(AggregateError) | ||
| if (error instanceof AggregateError) { | ||
| expect(error.errors).toHaveLength(2) | ||
| } | ||
| }) | ||
| }) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
Uh oh!
There was an error while loading. Please reload this page.