-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
breaking: tighten up error handling #11289
Merged
Merged
Changes from 9 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
64565c0
breaking: tighten up error handling
dummdidumm fddc54f
how did this happen
dummdidumm 7613649
fix tests
dummdidumm 5bf57fb
lint
dummdidumm 34acad2
Merge branch 'version-2' into tighten-up-error-handling
dummdidumm ae79081
Merge branch 'version-2' into tighten-up-error-handling
dummdidumm 067c3fa
types
dummdidumm 43e7d2e
Merge branch 'version-2' into tighten-up-error-handling
dummdidumm a5da208
adjust wording (is this even a breaking change now?)
dummdidumm 8f0fbab
adjust
dummdidumm 6d77aa5
pass status and message to handleError
dummdidumm c1c280c
merge
Rich-Harris da81262
Apply suggestions from code review
dummdidumm e4c96b7
lint
dummdidumm e5ad6f1
Update documentation/docs/30-advanced/20-hooks.md
Rich-Harris a291b70
Merge branch 'version-2' into tighten-up-error-handling
Rich-Harris f75abdc
lint
Rich-Harris bbe2c5d
simplify example
Rich-Harris 7f5df13
tweak docs
Rich-Harris 877605f
Update documentation/docs/30-advanced/20-hooks.md
Rich-Harris 99b54e4
Merge branch 'tighten-up-error-handling' of github.com:sveltejs/kit i…
Rich-Harris 824198d
various tweaks. we can be less duplicative i think
Rich-Harris 7ecbca5
tweak
Rich-Harris 24bdfd9
tweak
Rich-Harris c1265d8
handle too large body after streaming has started
Rich-Harris 452e264
cancel stream from the inside if content-length exceeds limit
Rich-Harris 5a914a2
remove unnecessary try-catch, bump adapter-node/adapter-vercel majors
Rich-Harris 95bb358
migration docs
Rich-Harris b24e8eb
tweak/fix tests
Rich-Harris d31afbb
fix more
Rich-Harris 231f212
more
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 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,5 @@ | ||
--- | ||
"@sveltejs/kit": major | ||
--- | ||
|
||
breaking: tighten up error handling |
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
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
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
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
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
Oops, something went wrong.
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.
Why doesn't this use
SvelteKitError
?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.
I don't think an error at this stage will be handled by
handelError
, so I kept this clear of it. But we could use it.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.
Not at this stage, no. But the thing that reads the body will either catch the error (in which case it is then responsible for setting a status code) or, more likely, the error will go uncaught. At that point, it's useful if the error was a
SvelteKitError
with a correct status code. If it's a POJO thenhandleError
will be called with a 500 Internal Error.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.
More correctly: the above applied to the second occurrence, once the stream has already started being read.
In this case, we're still creating the
Request
object. Right now, every usage ofgetRequest
has to be wrapped in atry-catch
, and thecatch
clause always looks like this:This makes no sense — there's only one possible error (the 413), and the diagnostic message gets swallowed.
I think the best solution is to throw the 'content length exceeds limit' error immediately when the stream starts. That way,
getRequest
callers don't need totry-catch
, and we get both user-friendly and developer-friendly error messages.