feat(feedback): Add level and remove breadcrumbs from feedback event
#9533
Conversation
Also adds a browser integration test
size-limit report 📦
|
| @@ -39,5 +38,8 @@ export async function prepareFeedbackEvent({ | |||
| // we need to do this manually. | |||
| preparedEvent.platform = preparedEvent.platform || 'javascript'; | |||
|
|
|||
| // No use for breadcrumbs in feedb | |||
There was a problem hiding this comment.
| // No use for breadcrumbs in feedb | |
| // No use for breadcrumbs in feedback |
| if (source === FEEDBACK_WIDGET_SOURCE) { | ||
| scope.setLevel('info'); | ||
| } |
There was a problem hiding this comment.
This is not good because we mutate the scope, so that will also affect other events that have the same scope (unless I am missing something and we are always forking a scope for this scenario?). So we should do one of these:
- Fork a scope here, and mutate that one:
withScope(scope =>
scope.setLevel(...);
prepareFeedbackEvent({ scope, ... })
})- put the level directly on the feedback event somehow
There was a problem hiding this comment.
Good catch! I updated to use pushScope and popScope -- I don't love the withScope API.... I wish withScope returned the new scope.
| @@ -34,58 +34,28 @@ export async function sendFeedbackRequest({ | |||
| type: 'feedback', | |||
| }; | |||
|
|
|||
| // Create a new scope | |||
| const scope = hub.pushScope(); | |||
There was a problem hiding this comment.
I'd recommend to use withScope instead (we'll probably get rid of pushScope in v8):
withScope((scope) => {
scope.clearBreadcrumbs();
if ([FEEDBACK_API_SOURCE, FEEDBACK_WIDGET_SOURCE].includes(String(source))) {
scope.setLevel('info');
}
const feedbackEvent = ...
});There was a problem hiding this comment.
Updated using withScope, had to create a promise because I need the return value inside of withScope callback
| return new Promise((resolve, reject) => { | ||
| hub.withScope(async scope => { | ||
| // No use for breadcrumbs in feedback | ||
| scope.clearBreadcrumbs(); |
There was a problem hiding this comment.
just a note, nothing to do here, but this may change a bit in v8. We're thinking to rework how/where we keep breadcrumbs, in the browser we'll probably keep them globally. At this point we'll have to change this logic here. Feel free to leave this for later, but if there is an easy way to make this forwards-compatible, we may as well do it already now 👍
There was a problem hiding this comment.
Good to know, would it be better to delete breadcrumbs from the event rather than the scope?
Also adds a browser integration test as well