From 1b871adecda69a149e20ad44c568ea4c45e35c6e Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 15 Dec 2023 13:07:23 +0100 Subject: [PATCH 1/2] feat(core): Update `withScope` to return callback return value To align this with OpenTelemetry and make some things possible that are currently not easily doable without `pushScope` / `popScope`. --- packages/core/src/exports.ts | 4 +- packages/core/src/hub.ts | 4 +- packages/core/test/lib/exports.test.ts | 53 ++++++++++++++++++++++++++ packages/types/src/hub.ts | 2 +- 4 files changed, 58 insertions(+), 5 deletions(-) create mode 100644 packages/core/test/lib/exports.test.ts diff --git a/packages/core/src/exports.ts b/packages/core/src/exports.ts index 0e574c4853cc..c5ab1055e7f4 100644 --- a/packages/core/src/exports.ts +++ b/packages/core/src/exports.ts @@ -163,8 +163,8 @@ export function setUser(user: User | null): ReturnType { * * @param callback that will be enclosed into push/popScope. */ -export function withScope(callback: (scope: Scope) => void): ReturnType { - getCurrentHub().withScope(callback); +export function withScope(callback: (scope: Scope) => T): T { + return getCurrentHub().withScope(callback); } /** diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index 13d5fd059e93..b7cffed42902 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -161,10 +161,10 @@ export class Hub implements HubInterface { /** * @inheritDoc */ - public withScope(callback: (scope: Scope) => void): void { + public withScope(callback: (scope: Scope) => T): T { const scope = this.pushScope(); try { - callback(scope); + return callback(scope); } finally { this.popScope(); } diff --git a/packages/core/test/lib/exports.test.ts b/packages/core/test/lib/exports.test.ts new file mode 100644 index 000000000000..3e4fed1f4fd0 --- /dev/null +++ b/packages/core/test/lib/exports.test.ts @@ -0,0 +1,53 @@ +import { Hub, Scope, makeMain, withScope, getCurrentScope } from '../../src'; +import { TestClient, getDefaultTestClientOptions } from '../mocks/client'; + +function getTestClient(): TestClient { + return new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://username@domain/123', + }), + ); +} + +describe('withScope', () => { + beforeEach(() => { + const client = getTestClient(); + const hub = new Hub(client); + makeMain(hub); + }); + + it('works without a return value', () => { + const scope1 = getCurrentScope(); + expect(scope1).toBeInstanceOf(Scope); + + scope1.setTag('foo', 'bar'); + + const res = withScope(scope => { + expect(scope).toBeInstanceOf(Scope); + expect(scope).not.toBe(scope1); + expect(scope['_tags']).toEqual({ foo: 'bar' }); + + expect(getCurrentScope()).toBe(scope); + }); + + expect(getCurrentScope()).toBe(scope1); + expect(res).toBe(undefined); + }); + + it('works with a return value', () => { + const res = withScope(scope => { + return 'foo'; + }); + + expect(res).toBe('foo'); + }); + + it('works with an async function',async () => { + const res = withScope(async scope => { + return 'foo'; + }); + + expect(res).toBeInstanceOf(Promise); + expect(await res).toBe('foo') + }); +}); diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index b7649cede039..a9c87cc157b7 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -65,7 +65,7 @@ export interface Hub { * * @param callback that will be enclosed into push/popScope. */ - withScope(callback: (scope: Scope) => void): void; + withScope(callback: (scope: Scope) => T): T; /** Returns the client of the top stack. */ getClient(): Client | undefined; From c9e82ece5d16e032bf67f6143f2d9e62ed2f4601 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 15 Dec 2023 14:19:54 +0100 Subject: [PATCH 2/2] minor feedback refactor to fix eslint --- packages/core/test/lib/exports.test.ts | 6 +- .../feedback/src/util/sendFeedbackRequest.ts | 91 +++++++++---------- 2 files changed, 44 insertions(+), 53 deletions(-) diff --git a/packages/core/test/lib/exports.test.ts b/packages/core/test/lib/exports.test.ts index 3e4fed1f4fd0..89b4fd9105d5 100644 --- a/packages/core/test/lib/exports.test.ts +++ b/packages/core/test/lib/exports.test.ts @@ -1,4 +1,4 @@ -import { Hub, Scope, makeMain, withScope, getCurrentScope } from '../../src'; +import { Hub, Scope, getCurrentScope, makeMain, withScope } from '../../src'; import { TestClient, getDefaultTestClientOptions } from '../mocks/client'; function getTestClient(): TestClient { @@ -42,12 +42,12 @@ describe('withScope', () => { expect(res).toBe('foo'); }); - it('works with an async function',async () => { + it('works with an async function', async () => { const res = withScope(async scope => { return 'foo'; }); expect(res).toBeInstanceOf(Promise); - expect(await res).toBe('foo') + expect(await res).toBe('foo'); }); }); diff --git a/packages/feedback/src/util/sendFeedbackRequest.ts b/packages/feedback/src/util/sendFeedbackRequest.ts index 5e8e532ca58d..f1629a00670a 100644 --- a/packages/feedback/src/util/sendFeedbackRequest.ts +++ b/packages/feedback/src/util/sendFeedbackRequest.ts @@ -33,67 +33,58 @@ export async function sendFeedbackRequest( type: 'feedback', }; - return new Promise((resolve, reject) => { - withScope(async scope => { - // No use for breadcrumbs in feedback - scope.clearBreadcrumbs(); - - if ([FEEDBACK_API_SOURCE, FEEDBACK_WIDGET_SOURCE].includes(String(source))) { - scope.setLevel('info'); - } + return withScope(async scope => { + // No use for breadcrumbs in feedback + scope.clearBreadcrumbs(); + + if ([FEEDBACK_API_SOURCE, FEEDBACK_WIDGET_SOURCE].includes(String(source))) { + scope.setLevel('info'); + } + + const feedbackEvent = await prepareFeedbackEvent({ + scope, + client, + event: baseEvent, + }); - const feedbackEvent = await prepareFeedbackEvent({ - scope, - client, - event: baseEvent, - }); + if (!feedbackEvent) { + return; + } - if (!feedbackEvent) { - resolve(); - return; - } + if (client.emit) { + client.emit('beforeSendFeedback', feedbackEvent, { includeReplay: Boolean(includeReplay) }); + } - if (client.emit) { - client.emit('beforeSendFeedback', feedbackEvent, { includeReplay: Boolean(includeReplay) }); - } + const envelope = createEventEnvelope(feedbackEvent, dsn, client.getOptions()._metadata, client.getOptions().tunnel); - const envelope = createEventEnvelope( - feedbackEvent, - dsn, - client.getOptions()._metadata, - client.getOptions().tunnel, - ); + let response: void | TransportMakeRequestResponse; - let response: void | TransportMakeRequestResponse; + try { + response = await transport.send(envelope); + } catch (err) { + const error = new Error('Unable to send Feedback'); try { - response = await transport.send(envelope); - } catch (err) { - const error = new Error('Unable to send Feedback'); - - try { - // In case browsers don't allow this property to be writable - // @ts-expect-error This needs lib es2022 and newer - error.cause = err; - } catch { - // nothing to do - } - reject(error); + // In case browsers don't allow this property to be writable + // @ts-expect-error This needs lib es2022 and newer + error.cause = err; + } catch { + // nothing to do } + throw error; + } - // TODO (v8): we can remove this guard once transport.send's type signature doesn't include void anymore - if (!response) { - resolve(response); - return; - } + // TODO (v8): we can remove this guard once transport.send's type signature doesn't include void anymore + if (!response) { + return; + } - // Require valid status codes, otherwise can assume feedback was not sent successfully - if (typeof response.statusCode === 'number' && (response.statusCode < 200 || response.statusCode >= 300)) { - reject(new Error('Unable to send Feedback')); - } + // Require valid status codes, otherwise can assume feedback was not sent successfully + if (typeof response.statusCode === 'number' && (response.statusCode < 200 || response.statusCode >= 300)) { + throw new Error('Unable to send Feedback'); + } - resolve(response); - }); + return response; }); }