-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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(nextjs): Add Edge Runtime SDK #6752
Conversation
size-limit report 📦
|
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.
h: can we get some unit tests? we can just copy the one's we have in node.
focus should be one
a) the transport
b) _prepareEvent
call in the client (since that sets a edge runtime that is different)
|
||
const nodeStackParser = createStackParser(nodeStackLineParser()); | ||
|
||
export const defaultIntegrations = [new CoreIntegrations.InboundFilters(), new CoreIntegrations.FunctionToString()]; |
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.
In the future we can include RequestData
and LinkedErrors
here since they are agnostic to node, but that requires some more refactoring.
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.
LinkedErrors could probably even live in @sentry/core
. We currently duplicate it in browser & node. As for RequestData
I am a bit unsure. That integration has become a bit of a behemoth and I don't like it becoming even more generic.
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.
We currently duplicate it in browser & node
There is slightly different behaviour for node/browser 😅
RequestData
being overloaded is fair, we can probably re-examine this.
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.
CI is 3 fast 5 me
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.
Wow, amazing work man!
Just checked: For SvelteKit, users can also opt-in to use edge: https://github.com/sveltejs/kit/tree/master/packages/adapter-vercel
Just wondering if it makes sense to proactively put this SDK into its own package so we could use it from a potential SvelteKit SDK in the future. WDYT?
Although, if I'm not mistaken we should be able to do this in a non-breaking way in the future, right? In that case let's just leave it as is for now.
we can extract into another package at any time in the future, so let's leave it till we absolutely need it. For now we leave it in here. |
Nice to know! I guess we have some experience now on how to tackle this!
Yes, we can move everything into its own package afterward without breaking things. I agree with the idea of only doing this though when we get to it! |
The same applies for Remix: https://github.com/remix-run/remix/tree/main/packages/remix-vercel 😄 |
Ref: #4206
This PR adds a very small SDK that is exported by
@sentry/nexts
which is capable of running on the Vercel Edge Runtime.The errors that are captured through this SDK are still very barebones and we have no way of source mapping yet (we don't even know if it is possible).
Testing this change is impossible with our current setup. I will create proper E2E tests when #6746 is merged