From f9c71ff3a1826d5927ced89fd91d40c5f26bfa8d Mon Sep 17 00:00:00 2001
From: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
Date: Fri, 25 Jun 2021 14:43:32 +0200
Subject: [PATCH] feat: withScope returns from wrapped function

This is part of a larger work towards better scope management.

This change makes Sentry.withScope and Hub.withScope easier to use with
existing functions that return a non-void type.

The motivation is that withScope could be used to wrap existing
functions and run them in a new scope without requiring a closure or
mutating state outside of the wrapped function (since it had to be
void).

While this does change the withScope signature, we're considering it
relatively low risk of breaking downstream code as we're making it more
general.
---
 packages/hub/src/hub.ts                   |  4 ++--
 packages/hub/test/hub.test.ts             | 29 +++++++++++++++++++++--
 packages/minimal/src/index.ts             |  8 +++----
 packages/minimal/test/lib/minimal.test.ts |  4 +++-
 packages/types/src/hub.ts                 |  6 ++---
 5 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts
index 1b14f6f5b899..85078f460dd9 100644
--- a/packages/hub/src/hub.ts
+++ b/packages/hub/src/hub.ts
@@ -148,10 +148,10 @@ export class Hub implements HubInterface {
   /**
    * @inheritDoc
    */
-  public withScope(callback: (scope: Scope) => void): void {
+  public withScope<T>(fn: (scope: Scope) => T): T {
     const scope = this.pushScope();
     try {
-      callback(scope);
+      return fn(scope);
     } finally {
       this.popScope();
     }
diff --git a/packages/hub/test/hub.test.ts b/packages/hub/test/hub.test.ts
index 1608a3978fc6..2e84df5ef24d 100644
--- a/packages/hub/test/hub.test.ts
+++ b/packages/hub/test/hub.test.ts
@@ -121,8 +121,13 @@ describe('Hub', () => {
   });
 
   describe('withScope', () => {
+    let hub: Hub;
+
+    beforeEach(() => {
+      hub = new Hub();
+    });
+
     test('simple', () => {
-      const hub = new Hub();
       hub.withScope(() => {
         expect(hub.getStack()).toHaveLength(2);
       });
@@ -130,7 +135,6 @@ describe('Hub', () => {
     });
 
     test('bindClient', () => {
-      const hub = new Hub();
       const testClient: any = { bla: 'a' };
       hub.withScope(() => {
         hub.bindClient(testClient);
@@ -139,6 +143,27 @@ describe('Hub', () => {
       });
       expect(hub.getStack()).toHaveLength(1);
     });
+
+    test('should bubble up exceptions', () => {
+      const error = new Error('test');
+      expect(() => {
+        hub.withScope(() => {
+          throw error;
+        });
+      }).toThrow(error);
+    });
+
+    test('should return return value from wrapped function', () => {
+      // someFn represents an existing function
+      const someFn = () => {
+        const hub = getCurrentHub();
+        hub.setTag('key', 'value');
+        hub.captureMessage('test');
+        return 'ok';
+      };
+      const value = hub.withScope(someFn); // runs someFn in a new scope
+      expect(value).toBe('ok');
+    });
   });
 
   test('getCurrentClient', () => {
diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts
index 117c8a769b92..f0039a2e187d 100644
--- a/packages/minimal/src/index.ts
+++ b/packages/minimal/src/index.ts
@@ -170,13 +170,13 @@ export function setUser(user: User | null): void {
  * This is essentially a convenience function for:
  *
  *     pushScope();
- *     callback();
+ *     fn();
  *     popScope();
  *
- * @param callback that will be enclosed into push/popScope.
+ * @param fn wrapped function.
  */
-export function withScope(callback: (scope: Scope) => void): void {
-  callOnHub<void>('withScope', callback);
+export function withScope<T>(fn: (scope: Scope) => T): T {
+  return callOnHub<T>('withScope', fn);
 }
 
 /**
diff --git a/packages/minimal/test/lib/minimal.test.ts b/packages/minimal/test/lib/minimal.test.ts
index 9e25bb0a46df..1f5e57e2c6c4 100644
--- a/packages/minimal/test/lib/minimal.test.ts
+++ b/packages/minimal/test/lib/minimal.test.ts
@@ -244,7 +244,7 @@ describe('Minimal', () => {
   });
 
   test('withScope', () => {
-    withScope(scope => {
+    const value = withScope(scope => {
       scope.setLevel(Severity.Warning);
       scope.setFingerprint(['1']);
       withScope(scope2 => {
@@ -261,8 +261,10 @@ describe('Minimal', () => {
         expect(global.__SENTRY__.hub._stack).toHaveLength(3);
       });
       expect(global.__SENTRY__.hub._stack).toHaveLength(2);
+      return 'ok';
     });
     expect(global.__SENTRY__.hub._stack).toHaveLength(1);
+    expect(value).toBe('ok');
   });
 
   test('setExtras', () => {
diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts
index a6be3e203db6..ed39d26cd219 100644
--- a/packages/types/src/hub.ts
+++ b/packages/types/src/hub.ts
@@ -61,12 +61,12 @@ export interface Hub {
    * This is essentially a convenience function for:
    *
    *     pushScope();
-   *     callback();
+   *     fn();
    *     popScope();
    *
-   * @param callback that will be enclosed into push/popScope.
+   * @param fn wrapped function.
    */
-  withScope(callback: (scope: Scope) => void): void;
+  withScope<T>(fn: (scope: Scope) => T): T;
 
   /** Returns the client of the top stack. */
   getClient(): Client | undefined;