-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(sveltekit): Add performance monitoring to Sveltekit server handle #7532
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
Conversation
size-limit report 📦
|
Playing around with wiring up https://github.com/sveltejs/sites/tree/master/sites/hn.svelte.dev We'll need server-side fetch support for this though (I locally did monkeypatching to get the |
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.
Looks good to me!
Just had a comment about tests.
expect(e.message).toEqual(type); | ||
} | ||
|
||
if (!isError) { |
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.
m: hmm not really a fan of conditional paths in tests. Makes tests harder to understand and also feels like we're testing different scenarios in one test. To me (maybe just personal preference), it seems like we're misusing each
here a little. That being said, if we split the isError
cases to separate tests, we probably end up with a ton of duplicated code, right?
I'll leave this up to you: If refactoring these tests results in too much duplication or work, I'm fine with leaving it as is. Generally though, let's try to avoid conditional expects and test paths.
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.
Yeah I struggled with this a little, but one of the issues here is that there are 4 different scenarios we need to account for.
- Async and no error
- Async and throw an error
- Sync and no error
- Sync and throws no error
Let me try cleaning it up so we can remove the isError
checks.
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.
ref #7526
In this PR we start tracking the performance of the SvelteKit server-side routes via the
handle
function.If you do use a
sequence
function, you should place thesentryHandle
as your first so that the transaction duration for the api request is as accurate as possible.