-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add missing timeout dev configuration for functions #346
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 7 commits
b3a0d2e
06806c6
6a8f71f
5f61ad6
de3f0eb
945ab24
9f7d20f
38447dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import { describe, expect, test } from 'vitest' | ||
|
||
import { BACKGROUND_FUNCTION_TIMEOUT, getFunctionTimeout, SYNCHRONOUS_FUNCTION_TIMEOUT } from './consts.js' | ||
|
||
describe('Function timeout constants', () => { | ||
test('exports correct timeout values', () => { | ||
expect(SYNCHRONOUS_FUNCTION_TIMEOUT).toBe(30) | ||
expect(BACKGROUND_FUNCTION_TIMEOUT).toBe(900) | ||
}) | ||
}) | ||
|
||
describe('getFunctionTimeout', () => { | ||
test('returns default timeout for synchronous functions when no site config', () => { | ||
const result = getFunctionTimeout({}) | ||
expect(result).toBe(SYNCHRONOUS_FUNCTION_TIMEOUT) | ||
}) | ||
|
||
test('returns default timeout for background functions when no site config', () => { | ||
const result = getFunctionTimeout({}, true) | ||
expect(result).toBe(BACKGROUND_FUNCTION_TIMEOUT) | ||
}) | ||
|
||
test('respects functionsTimeout from site config for synchronous functions', () => { | ||
const siteConfig = { functionsTimeout: 60 } | ||
const result = getFunctionTimeout(siteConfig) | ||
expect(result).toBe(60) | ||
}) | ||
|
||
test('respects functionsTimeout from site config for background functions', () => { | ||
const siteConfig = { functionsTimeout: 1800 } | ||
const result = getFunctionTimeout(siteConfig, true) | ||
expect(result).toBe(1800) | ||
}) | ||
|
||
test('respects functionsConfig.timeout from site config for synchronous functions', () => { | ||
const siteConfig = { functionsConfig: { timeout: 45 } } | ||
const result = getFunctionTimeout(siteConfig) | ||
expect(result).toBe(45) | ||
}) | ||
|
||
test('respects functionsConfig.timeout from site config for background functions', () => { | ||
const siteConfig = { functionsConfig: { timeout: 1200 } } | ||
const result = getFunctionTimeout(siteConfig, true) | ||
expect(result).toBe(1200) | ||
}) | ||
|
||
test('prioritizes functionsTimeout over functionsConfig.timeout', () => { | ||
const siteConfig = { | ||
functionsTimeout: 60, | ||
functionsConfig: { timeout: 45 }, | ||
} | ||
const result = getFunctionTimeout(siteConfig) | ||
expect(result).toBe(60) | ||
}) | ||
|
||
test('handles undefined functionsConfig', () => { | ||
const siteConfig = { functionsConfig: undefined } | ||
const result = getFunctionTimeout(siteConfig) | ||
expect(result).toBe(SYNCHRONOUS_FUNCTION_TIMEOUT) | ||
}) | ||
|
||
test('handles empty functionsConfig object', () => { | ||
const siteConfig = { functionsConfig: {} } | ||
const result = getFunctionTimeout(siteConfig) | ||
expect(result).toBe(SYNCHRONOUS_FUNCTION_TIMEOUT) | ||
}) | ||
|
||
test('handles undefined timeout in functionsConfig', () => { | ||
const siteConfig = { functionsConfig: { timeout: undefined } } | ||
const result = getFunctionTimeout(siteConfig) | ||
expect(result).toBe(SYNCHRONOUS_FUNCTION_TIMEOUT) | ||
}) | ||
|
||
test('handles zero timeout values', () => { | ||
const siteConfig = { functionsTimeout: 0 } | ||
const result = getFunctionTimeout(siteConfig) | ||
expect(result).toBe(0) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,43 @@ | ||
import type { SiteConfig } from '@netlify/types' | ||
|
||
const BUILDER_FUNCTIONS_FLAG = true | ||
const HTTP_STATUS_METHOD_NOT_ALLOWED = 405 | ||
const HTTP_STATUS_OK = 200 | ||
const METADATA_VERSION = 1 | ||
|
||
export { BUILDER_FUNCTIONS_FLAG, HTTP_STATUS_METHOD_NOT_ALLOWED, HTTP_STATUS_OK, METADATA_VERSION } | ||
/** | ||
* Default timeout for synchronous functions in seconds | ||
*/ | ||
const SYNCHRONOUS_FUNCTION_TIMEOUT = 30 | ||
|
||
/** | ||
* Default timeout for background functions in seconds | ||
*/ | ||
const BACKGROUND_FUNCTION_TIMEOUT = 900 | ||
|
||
/** | ||
* Get the effective function timeout considering site-specific configuration | ||
* @param siteConfig - Site configuration with timeout options | ||
* @param isBackground - Whether this is a background function | ||
* @returns Function timeout in seconds | ||
*/ | ||
export function getFunctionTimeout(siteConfig: SiteConfig, isBackground = false): number { | ||
// Check for site-specific timeout configuration | ||
const siteTimeout = siteConfig.functionsTimeout ?? siteConfig.functionsConfig?.timeout | ||
|
||
if (siteTimeout !== undefined) { | ||
return siteTimeout | ||
} | ||
|
||
// Use default timeout based on function type | ||
return isBackground ? BACKGROUND_FUNCTION_TIMEOUT : SYNCHRONOUS_FUNCTION_TIMEOUT | ||
} | ||
|
||
export { | ||
BUILDER_FUNCTIONS_FLAG, | ||
HTTP_STATUS_METHOD_NOT_ALLOWED, | ||
HTTP_STATUS_OK, | ||
METADATA_VERSION, | ||
SYNCHRONOUS_FUNCTION_TIMEOUT, | ||
BACKGROUND_FUNCTION_TIMEOUT, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,3 +3,4 @@ export { purgeCache } from './lib/purge_cache.js' | |
export { schedule } from './lib/schedule.js' | ||
export { stream } from './lib/stream.js' | ||
export * from './function/index.js' | ||
export { SYNCHRONOUS_FUNCTION_TIMEOUT, BACKGROUND_FUNCTION_TIMEOUT, getFunctionTimeout } from './lib/consts.js' | ||
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. there's no need to export these 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. Removed the unnecessary exports from main.ts. The timeout logic is now properly contained within the functions where it's used. Commit: b4f8e3e |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
import { describe, expect, expectTypeOf, test } from 'vitest' | ||
|
||
import type { Site, SiteConfig } from './site.js' | ||
|
||
describe('Site types', () => { | ||
test('Site interface accepts all optional properties', () => { | ||
const site: Site = {} | ||
expect(site).toBeDefined() | ||
|
||
const siteWithProps: Site = { | ||
id: 'site-id', | ||
name: 'My Site', | ||
url: 'https://example.com', | ||
} | ||
expect(siteWithProps.id).toBe('site-id') | ||
expect(siteWithProps.name).toBe('My Site') | ||
expect(siteWithProps.url).toBe('https://example.com') | ||
}) | ||
|
||
test('SiteConfig interface accepts optional timeout properties', () => { | ||
const config: SiteConfig = {} | ||
expect(config).toBeDefined() | ||
|
||
const configWithFunctionsTimeout: SiteConfig = { | ||
functionsTimeout: 60, | ||
} | ||
expect(configWithFunctionsTimeout.functionsTimeout).toBe(60) | ||
|
||
const configWithFunctionsConfig: SiteConfig = { | ||
functionsConfig: { | ||
timeout: 45, | ||
}, | ||
} | ||
expect(configWithFunctionsConfig.functionsConfig?.timeout).toBe(45) | ||
|
||
const configWithBoth: SiteConfig = { | ||
functionsTimeout: 60, | ||
functionsConfig: { | ||
timeout: 45, | ||
}, | ||
} | ||
expect(configWithBoth.functionsTimeout).toBe(60) | ||
expect(configWithBoth.functionsConfig?.timeout).toBe(45) | ||
}) | ||
|
||
test('timeout values have correct types', () => { | ||
const config: SiteConfig = { | ||
functionsTimeout: 30, | ||
functionsConfig: { | ||
timeout: 900, | ||
}, | ||
} | ||
|
||
expect(config.functionsTimeout).toBe(30) | ||
expect(config.functionsConfig?.timeout).toBe(900) | ||
|
||
expectTypeOf(config.functionsTimeout).toEqualTypeOf<number | undefined>() | ||
expectTypeOf(config.functionsConfig).toEqualTypeOf<{ timeout?: number } | undefined>() | ||
|
||
if (config.functionsTimeout) { | ||
expectTypeOf(config.functionsTimeout).toBeNumber() | ||
} | ||
|
||
if (config.functionsConfig?.timeout) { | ||
expectTypeOf(config.functionsConfig.timeout).toBeNumber() | ||
} | ||
}) | ||
}) |
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.
this is a file called
consts.ts
so I'm not convinced it should export functions. let's move this to where it's used instead.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.
Moved the getFunctionTimeout function from consts.ts to where it's actually used in the registry and main dev files. Functions stay in consts.ts, behavior moves to where it's needed. Commit: b4f8e3e