-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Error handling fix #5314
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
Merged
Merged
Error handling fix #5314
Changes from all commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
c9c152e
fix and almost working tests
cdcarson 48ddd8c
todo note in failing test
cdcarson 5ecfc93
tests working
cdcarson bfa3dec
formatting
cdcarson f72ddd1
added changeset
cdcarson adaf85e
Merge branch 'master' into error-handling-fix
Rich-Harris ec052ea
Merge branch 'master' into error-handling-fix
Rich-Harris 2e41c78
make tests more consistent with the rest of the codebase
Rich-Harris 9d0fbcf
simplify tests
Rich-Harris 5216d42
include stack trace, tweak tests a bit
Rich-Harris f7f9fc2
update changeset
Rich-Harris 1852bbe
oh do fuck off windows
Rich-Harris 618e16e
shuffle things around a bit, ensure handleError is called
Rich-Harris e4f886b
add (failing) tests for explicit errors
Rich-Harris 30fd42f
update tests
Rich-Harris ec1cdde
render error page if body instanceof Error
Rich-Harris bb1c710
preserve errors returned from page endpoint GET handlers
Rich-Harris 3c8360e
serialize errors consistently
Rich-Harris 3a2127f
better error serialization
Rich-Harris c8b9feb
beef up tests
Rich-Harris 2694afa
remove test.only
Rich-Harris 75e69e1
reuse serialize_error
Rich-Harris 3940383
shut up eslint, you big dummy
Rich-Harris 091c09f
bah typescript
Rich-Harris 91aa2dd
stack is already fixed
Rich-Harris 01fdc7e
explicitly add Error to ResponseBody
Rich-Harris fcbb899
overhaul endpoint docs to mention Error
Rich-Harris ca762d8
more doc tweaks
Rich-Harris b19c3f5
Update packages/kit/src/runtime/server/utils.spec.js
Rich-Harris 402ae5c
add comments
Rich-Harris 2458f3a
Merge branch 'error-handling-fix' of github.com:cdcarson/kit into err…
Rich-Harris 83c6252
DRY some stuff out
Rich-Harris 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 hidden or 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 |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@sveltejs/kit': patch | ||
| 'test-basics': patch | ||
| --- | ||
|
|
||
| Returns errors from page endpoints as JSON where appropriate |
This file contains hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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
Oops, something went wrong.
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.
could refactor this into a reusable function since the other new code added here does the same thing
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.
feels like unnecessary indirection to me — you add function call overhead, a new utility somewhere in the codebase, an import declaration per consuming module... you shave off a few characters at the callsite but make the codebase larger and arguably less easy to read
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.
ah, you're talking about the whole block, not just the highlighted line. thought you mean
is_error(body)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 one of those cases where you basically can't DRY it out, because of the control flow (i.e. mutating an existing object and conditionally returning it). Or rather you can, but the resulting function is larger than the code you saved by deduplicating. i reckon it's probably not worth it