-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: propagate onError config property to manifest #328
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
a1432d7
fix: throw errors when function or isc-config cannot be loaded
danez 358dead
chore: fix FF type
danez d930755
feat: add on_error field
Skn0tt b2fba77
Merge branch 'main' into add-onerror-prop
Skn0tt 7db6740
:old-man-yells-at-linter:
Skn0tt b0e5370
fix: prettier
Skn0tt 088fe78
update snapshots
Skn0tt a704875
feat: move on_error into per-function config
Skn0tt 68fa355
fix: prettier, argh
Skn0tt b63bd00
Update node/config.ts
Skn0tt 339452b
fix: test
Skn0tt d2675ff
Merge branch 'main' into add-onerror-prop
Skn0tt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,10 +27,19 @@ export const enum Cache { | |
Manual = 'manual', | ||
} | ||
|
||
export type OnError = 'fail' | 'bypass' | `/${string}` | ||
|
||
export const isValidOnError = (value: unknown): value is OnError => { | ||
if (typeof value === 'undefined') return true | ||
if (typeof value !== 'string') return false | ||
return value === 'fail' || value === 'bypass' || value.startsWith('/') | ||
} | ||
|
||
export interface FunctionConfig { | ||
cache?: Cache | ||
path?: string | string[] | ||
excludedPath?: string | string[] | ||
onError?: OnError | ||
} | ||
|
||
const getConfigExtractor = () => { | ||
|
@@ -89,17 +98,26 @@ export const getFunctionConfig = async ( | |
log.user(stdout) | ||
} | ||
|
||
try { | ||
const collectorData = await fs.readFile(collector.path, 'utf8') | ||
let collectorData: FunctionConfig = {} | ||
|
||
return JSON.parse(collectorData) as FunctionConfig | ||
try { | ||
const collectorDataJSON = await fs.readFile(collector.path, 'utf8') | ||
collectorData = JSON.parse(collectorDataJSON) as FunctionConfig | ||
} catch { | ||
handleConfigError(func, ConfigExitCode.UnhandledError, stderr, log, featureFlags) | ||
|
||
return {} | ||
} finally { | ||
await collector.cleanup() | ||
} | ||
|
||
if (!isValidOnError(collectorData.onError)) { | ||
throw new BundleError( | ||
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. since this is new behaviour, I think it's fine to throw on error - no need to use the flag. |
||
new Error( | ||
`The 'onError' configuration property in edge function at '${func.path}' must be one of 'fail', 'bypass', or a path starting with '/'. Got '${collectorData.onError}'. More on the Edge Functions API at https://ntl.fyi/edge-api.`, | ||
), | ||
) | ||
} | ||
|
||
return collectorData | ||
} | ||
|
||
const handleConfigError = ( | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Curious why you're not just checking
value === undefined
?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.
age-old insecurities about how JS handles
null
vsundefined
. blame brendan eich