From b285cc0e28ba629c8c40604cbc0f6bfd2fbdf2d7 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 30 Nov 2021 12:37:11 -0500 Subject: [PATCH 1/8] ref(hub): Deprecate _invokeClient --- packages/hub/src/hub.ts | 30 ++++++++++++++++------- packages/hub/test/hub.test.ts | 22 ++++++++--------- packages/minimal/src/index.ts | 7 ++++++ packages/minimal/test/lib/minimal.test.ts | 1 + 4 files changed, 40 insertions(+), 20 deletions(-) diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 2c1f6b297faf..37e39c1f5d77 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -206,9 +206,8 @@ export class Hub implements HubInterface { }; } - this._invokeClient('captureException', exception, { - ...finalHint, - event_id: eventId, + this._withClient((client, scope) => { + client.captureException(exception, { ...finalHint, event_id: eventId }, scope); }); return eventId; } @@ -237,9 +236,8 @@ export class Hub implements HubInterface { }; } - this._invokeClient('captureMessage', message, level, { - ...finalHint, - event_id: eventId, + this._withClient((client, scope) => { + client.captureMessage(message, level, { ...finalHint, event_id: eventId }, scope); }); return eventId; } @@ -253,9 +251,8 @@ export class Hub implements HubInterface { this._lastEventId = eventId; } - this._invokeClient('captureEvent', event, { - ...hint, - event_id: eventId, + this._withClient((client, scope) => { + client.captureEvent(event, { ...hint, event_id: eventId }, scope); }); return eventId; } @@ -478,6 +475,7 @@ export class Hub implements HubInterface { * * @param method The method to call on the client. * @param args Arguments to pass to the client function. + * @deprecated */ // eslint-disable-next-line @typescript-eslint/no-explicit-any private _invokeClient(method: M, ...args: any[]): void { @@ -488,6 +486,20 @@ export class Hub implements HubInterface { } } + /** + * Internal helper function to call a method on the top client if it exists. + * + * @param method The method to call on the client. + * @param args Arguments to pass to the client function. + */ + // eslint-disable-next-line @typescript-eslint/no-explicit-any + private _withClient(callback: (client: Client, scope: Scope | undefined) => void): void { + const { scope, client } = this.getStackTop(); + if (client) { + callback(client, scope); + } + } + /** * Calls global extension method and binding current instance to the function call */ diff --git a/packages/hub/test/hub.test.ts b/packages/hub/test/hub.test.ts index 4e865fa10412..ed2fe132f366 100644 --- a/packages/hub/test/hub.test.ts +++ b/packages/hub/test/hub.test.ts @@ -27,7 +27,7 @@ describe('Hub', () => { expect(hub.getStack()).toHaveLength(1); }); - test("don't invoke client sync with wrong func", () => { + test.skip("don't invoke client sync with wrong func", () => { const hub = new Hub(clientFn); // @ts-ignore we want to able to call private method hub._invokeClient('funca', true); @@ -194,7 +194,7 @@ describe('Hub', () => { }); describe('captureException', () => { - test('simple', () => { + test.skip('simple', () => { const hub = new Hub(); const spy = jest.spyOn(hub as any, '_invokeClient'); hub.captureException('a'); @@ -203,7 +203,7 @@ describe('Hub', () => { expect(spy.mock.calls[0][1]).toBe('a'); }); - test('should set event_id in hint', () => { + test.skip('should set event_id in hint', () => { const hub = new Hub(); const spy = jest.spyOn(hub as any, '_invokeClient'); hub.captureException('a'); @@ -211,7 +211,7 @@ describe('Hub', () => { expect(spy.mock.calls[0][2].event_id).toBeTruthy(); }); - test('should generate hint if not provided in the call', () => { + test.skip('should generate hint if not provided in the call', () => { const hub = new Hub(); const spy = jest.spyOn(hub as any, '_invokeClient'); const ex = new Error('foo'); @@ -226,7 +226,7 @@ describe('Hub', () => { }); describe('captureMessage', () => { - test('simple', () => { + test.skip('simple', () => { const hub = new Hub(); const spy = jest.spyOn(hub as any, '_invokeClient'); hub.captureMessage('a'); @@ -235,7 +235,7 @@ describe('Hub', () => { expect(spy.mock.calls[0][1]).toBe('a'); }); - test('should set event_id in hint', () => { + test.skip('should set event_id in hint', () => { const hub = new Hub(); const spy = jest.spyOn(hub as any, '_invokeClient'); hub.captureMessage('a'); @@ -243,7 +243,7 @@ describe('Hub', () => { expect(spy.mock.calls[0][3].event_id).toBeTruthy(); }); - test('should generate hint if not provided in the call', () => { + test.skip('should generate hint if not provided in the call', () => { const hub = new Hub(); const spy = jest.spyOn(hub as any, '_invokeClient'); hub.captureMessage('foo'); @@ -257,7 +257,7 @@ describe('Hub', () => { }); describe('captureEvent', () => { - test('simple', () => { + test.skip('simple', () => { const event: Event = { extra: { b: 3 }, }; @@ -269,7 +269,7 @@ describe('Hub', () => { expect(spy.mock.calls[0][1]).toBe(event); }); - test('should set event_id in hint', () => { + test.skip('should set event_id in hint', () => { const event: Event = { extra: { b: 3 }, }; @@ -280,7 +280,7 @@ describe('Hub', () => { expect(spy.mock.calls[0][2].event_id).toBeTruthy(); }); - test('sets lastEventId', () => { + test.skip('sets lastEventId', () => { const event: Event = { extra: { b: 3 }, }; @@ -291,7 +291,7 @@ describe('Hub', () => { expect(spy.mock.calls[0][2].event_id).toEqual(hub.lastEventId()); }); - test('transactions do not set lastEventId', () => { + test.skip('transactions do not set lastEventId', () => { const event: Event = { extra: { b: 3 }, type: 'transaction', diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index 117c8a769b92..24cc8b4d2782 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -2,6 +2,7 @@ import { getCurrentHub, Hub, Scope } from '@sentry/hub'; import { Breadcrumb, CaptureContext, + Client, CustomSamplingContext, Event, Extra, @@ -188,12 +189,18 @@ export function withScope(callback: (scope: Scope) => void): void { * @param method The method to call on the client/client. * @param args Arguments to pass to the client/fontend. * @hidden + * @deprecated Please use {@link _withClient} */ // eslint-disable-next-line @typescript-eslint/no-explicit-any export function _callOnClient(method: string, ...args: any[]): void { callOnHub('_invokeClient', method, ...args); } +/** */ +export function _withClient(callback: (client: Client, scope: Scope | undefined) => void): void { + callOnHub('_withClient', callback); +} + /** * Starts a new `Transaction` and returns it. This is the entry point to manual tracing instrumentation. * diff --git a/packages/minimal/test/lib/minimal.test.ts b/packages/minimal/test/lib/minimal.test.ts index 9e25bb0a46df..2f5ac1e3ba9d 100644 --- a/packages/minimal/test/lib/minimal.test.ts +++ b/packages/minimal/test/lib/minimal.test.ts @@ -201,6 +201,7 @@ describe('Minimal', () => { const s = jest.spyOn(TestClient.prototype, 'mySecretPublicMethod'); getCurrentHub().withScope(() => { getCurrentHub().bindClient(new TestClient({}) as any); + // eslint-disable-next-line deprecation/deprecation _callOnClient('mySecretPublicMethod', 'test'); expect(s.mock.calls[0][0]).toBe('test'); s.mockRestore(); From 39bda632f572c46d121682e83c0800d55345c3fe Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 30 Nov 2021 12:49:25 -0500 Subject: [PATCH 2/8] Remove _invokeClient fr --- packages/hub/src/hub.ts | 16 ---------------- packages/minimal/src/index.ts | 16 ---------------- 2 files changed, 32 deletions(-) diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 37e39c1f5d77..0bae9f0b8ac6 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -470,22 +470,6 @@ export class Hub implements HubInterface { } } - /** - * Internal helper function to call a method on the top client if it exists. - * - * @param method The method to call on the client. - * @param args Arguments to pass to the client function. - * @deprecated - */ - // eslint-disable-next-line @typescript-eslint/no-explicit-any - private _invokeClient(method: M, ...args: any[]): void { - const { scope, client } = this.getStackTop(); - if (client && client[method]) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any - (client as any)[method](...args, scope); - } - } - /** * Internal helper function to call a method on the top client if it exists. * diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index 24cc8b4d2782..2dfe75024b6e 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -180,22 +180,6 @@ export function withScope(callback: (scope: Scope) => void): void { callOnHub('withScope', callback); } -/** - * Calls a function on the latest client. Use this with caution, it's meant as - * in "internal" helper so we don't need to expose every possible function in - * the shim. It is not guaranteed that the client actually implements the - * function. - * - * @param method The method to call on the client/client. - * @param args Arguments to pass to the client/fontend. - * @hidden - * @deprecated Please use {@link _withClient} - */ -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export function _callOnClient(method: string, ...args: any[]): void { - callOnHub('_invokeClient', method, ...args); -} - /** */ export function _withClient(callback: (client: Client, scope: Scope | undefined) => void): void { callOnHub('_withClient', callback); From 7a9319976f090e81981dd60f4862c951cd9fcfad Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 30 Nov 2021 12:50:17 -0500 Subject: [PATCH 3/8] delete test --- packages/minimal/test/lib/minimal.test.ts | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/packages/minimal/test/lib/minimal.test.ts b/packages/minimal/test/lib/minimal.test.ts index 2f5ac1e3ba9d..a3bf4c9e7de4 100644 --- a/packages/minimal/test/lib/minimal.test.ts +++ b/packages/minimal/test/lib/minimal.test.ts @@ -197,18 +197,6 @@ describe('Minimal', () => { expect(getCurrentHub().getClient()).toBe(TestClient.instance); }); - test('Calls function on the client', done => { - const s = jest.spyOn(TestClient.prototype, 'mySecretPublicMethod'); - getCurrentHub().withScope(() => { - getCurrentHub().bindClient(new TestClient({}) as any); - // eslint-disable-next-line deprecation/deprecation - _callOnClient('mySecretPublicMethod', 'test'); - expect(s.mock.calls[0][0]).toBe('test'); - s.mockRestore(); - done(); - }); - }); - test('does not throw an error when pushing different clients', () => { init({}); expect(() => { From ae1f4db57b13464382ef0d8f7b29f4e25491f0f7 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 30 Nov 2021 13:19:55 -0500 Subject: [PATCH 4/8] refactor tests --- packages/hub/test/hub.test.ts | 120 ++++++++++------------ packages/minimal/test/lib/minimal.test.ts | 1 - 2 files changed, 52 insertions(+), 69 deletions(-) diff --git a/packages/hub/test/hub.test.ts b/packages/hub/test/hub.test.ts index ed2fe132f366..becbcbffe591 100644 --- a/packages/hub/test/hub.test.ts +++ b/packages/hub/test/hub.test.ts @@ -27,13 +27,6 @@ describe('Hub', () => { expect(hub.getStack()).toHaveLength(1); }); - test.skip("don't invoke client sync with wrong func", () => { - const hub = new Hub(clientFn); - // @ts-ignore we want to able to call private method - hub._invokeClient('funca', true); - expect(clientFn).not.toHaveBeenCalled(); - }); - test('isOlderThan', () => { const hub = new Hub(); expect(hub.isOlderThan(0)).toBeFalsy(); @@ -194,113 +187,104 @@ describe('Hub', () => { }); describe('captureException', () => { - test.skip('simple', () => { - const hub = new Hub(); - const spy = jest.spyOn(hub as any, '_invokeClient'); + const mockClient: any = { captureException: jest.fn() }; + beforeEach(() => { + mockClient.captureException.mockClear(); + }); + + test('simple', () => { + const hub = new Hub(mockClient); hub.captureException('a'); - expect(spy).toHaveBeenCalled(); - expect(spy.mock.calls[0][0]).toBe('captureException'); - expect(spy.mock.calls[0][1]).toBe('a'); + expect(mockClient.captureException).toHaveBeenCalled(); + expect(mockClient.captureException.mock.calls[0][0]).toBe('a'); }); - test.skip('should set event_id in hint', () => { - const hub = new Hub(); - const spy = jest.spyOn(hub as any, '_invokeClient'); + test('should set event_id in hint', () => { + const hub = new Hub(mockClient); hub.captureException('a'); - // @ts-ignore Says mock object is type unknown - expect(spy.mock.calls[0][2].event_id).toBeTruthy(); + expect(mockClient.captureException.mock.calls[0][1].event_id).toBeTruthy(); }); - test.skip('should generate hint if not provided in the call', () => { - const hub = new Hub(); - const spy = jest.spyOn(hub as any, '_invokeClient'); + test('should generate hint if not provided in the call', () => { + const hub = new Hub(mockClient); const ex = new Error('foo'); hub.captureException(ex); - // @ts-ignore Says mock object is type unknown - expect(spy.mock.calls[0][2].originalException).toBe(ex); - // @ts-ignore Says mock object is type unknown - expect(spy.mock.calls[0][2].syntheticException).toBeInstanceOf(Error); - // @ts-ignore Says mock object is type unknown - expect(spy.mock.calls[0][2].syntheticException.message).toBe('Sentry syntheticException'); + expect(mockClient.captureException.mock.calls[0][1].originalException).toBe(ex); + expect(mockClient.captureException.mock.calls[0][1].syntheticException).toBeInstanceOf(Error); + expect(mockClient.captureException.mock.calls[0][1].syntheticException.message).toBe('Sentry syntheticException'); }); }); describe('captureMessage', () => { - test.skip('simple', () => { - const hub = new Hub(); - const spy = jest.spyOn(hub as any, '_invokeClient'); + const mockClient: any = { captureMessage: jest.fn() }; + beforeEach(() => { + mockClient.captureMessage.mockClear(); + }); + + test('simple', () => { + const hub = new Hub(mockClient); hub.captureMessage('a'); - expect(spy).toHaveBeenCalled(); - expect(spy.mock.calls[0][0]).toBe('captureMessage'); - expect(spy.mock.calls[0][1]).toBe('a'); + expect(mockClient.captureMessage).toHaveBeenCalled(); + expect(mockClient.captureMessage.mock.calls[0][0]).toBe('a'); }); - test.skip('should set event_id in hint', () => { - const hub = new Hub(); - const spy = jest.spyOn(hub as any, '_invokeClient'); + test('should set event_id in hint', () => { + const hub = new Hub(mockClient); hub.captureMessage('a'); - // @ts-ignore Says mock object is type unknown - expect(spy.mock.calls[0][3].event_id).toBeTruthy(); + expect(mockClient.captureMessage.mock.calls[0][2].event_id).toBeTruthy(); }); - test.skip('should generate hint if not provided in the call', () => { - const hub = new Hub(); - const spy = jest.spyOn(hub as any, '_invokeClient'); + test('should generate hint if not provided in the call', () => { + const hub = new Hub(mockClient); hub.captureMessage('foo'); - // @ts-ignore Says mock object is type unknown - expect(spy.mock.calls[0][3].originalException).toBe('foo'); - // @ts-ignore Says mock object is type unknown - expect(spy.mock.calls[0][3].syntheticException).toBeInstanceOf(Error); - // @ts-ignore Says mock object is type unknown - expect(spy.mock.calls[0][3].syntheticException.message).toBe('foo'); + expect(mockClient.captureMessage.mock.calls[0][2].originalException).toBe('foo'); + expect(mockClient.captureMessage.mock.calls[0][2].syntheticException).toBeInstanceOf(Error); + expect(mockClient.captureMessage.mock.calls[0][2].syntheticException.message).toBe('foo'); }); }); describe('captureEvent', () => { - test.skip('simple', () => { + const mockClient: any = { captureEvent: jest.fn() }; + beforeEach(() => { + mockClient.captureEvent.mockClear(); + }); + + test('simple', () => { const event: Event = { extra: { b: 3 }, }; - const hub = new Hub(); - const spy = jest.spyOn(hub as any, '_invokeClient'); + const hub = new Hub(mockClient); hub.captureEvent(event); - expect(spy).toHaveBeenCalled(); - expect(spy.mock.calls[0][0]).toBe('captureEvent'); - expect(spy.mock.calls[0][1]).toBe(event); + expect(mockClient.captureEvent).toHaveBeenCalled(); + expect(mockClient.captureEvent.mock.calls[0][0]).toBe(event); }); - test.skip('should set event_id in hint', () => { + test('should set event_id in hint', () => { const event: Event = { extra: { b: 3 }, }; - const hub = new Hub(); - const spy = jest.spyOn(hub as any, '_invokeClient'); + const hub = new Hub(mockClient); hub.captureEvent(event); - // @ts-ignore Says mock object is type unknown - expect(spy.mock.calls[0][2].event_id).toBeTruthy(); + expect(mockClient.captureEvent.mock.calls[0][1].event_id).toBeTruthy(); }); - test.skip('sets lastEventId', () => { + test('sets lastEventId', () => { const event: Event = { extra: { b: 3 }, }; - const hub = new Hub(); - const spy = jest.spyOn(hub as any, '_invokeClient'); + const hub = new Hub(mockClient); hub.captureEvent(event); - // @ts-ignore Says mock object is type unknown - expect(spy.mock.calls[0][2].event_id).toEqual(hub.lastEventId()); + expect(mockClient.captureEvent.mock.calls[0][1].event_id).toEqual(hub.lastEventId()); }); - test.skip('transactions do not set lastEventId', () => { + test('transactions do not set lastEventId', () => { const event: Event = { extra: { b: 3 }, type: 'transaction', }; - const hub = new Hub(); - const spy = jest.spyOn(hub as any, '_invokeClient'); + const hub = new Hub(mockClient); hub.captureEvent(event); - // @ts-ignore Says mock object is type unknown - expect(spy.mock.calls[0][2].event_id).not.toEqual(hub.lastEventId()); + expect(mockClient.captureEvent.mock.calls[0][1].event_id).not.toEqual(hub.lastEventId()); }); }); diff --git a/packages/minimal/test/lib/minimal.test.ts b/packages/minimal/test/lib/minimal.test.ts index a3bf4c9e7de4..c82a26faae74 100644 --- a/packages/minimal/test/lib/minimal.test.ts +++ b/packages/minimal/test/lib/minimal.test.ts @@ -2,7 +2,6 @@ import { getCurrentHub, getHubFromCarrier, Scope } from '@sentry/hub'; import { Severity } from '@sentry/types'; import { - _callOnClient, captureEvent, captureException, captureMessage, From fb557d113e59b49d07428287a501fd15d505da03 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 30 Nov 2021 13:23:41 -0500 Subject: [PATCH 5/8] Add docstring --- packages/minimal/src/index.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index 2dfe75024b6e..bd00e6233d9b 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -180,7 +180,14 @@ export function withScope(callback: (scope: Scope) => void): void { callOnHub('withScope', callback); } -/** */ +/** + * Executes a callback on the hub that is given a client and scope if they exist. If no client + * exists, the callback will not execute. + * + * @param callback that is called with client and scope. + * @hidden + */ +// eslint-disable-next-line @typescript-eslint/no-explicit-any export function _withClient(callback: (client: Client, scope: Scope | undefined) => void): void { callOnHub('_withClient', callback); } From bedbdc98772b857f6ccd552b98c8c6d54903e3a4 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 30 Nov 2021 13:24:35 -0500 Subject: [PATCH 6/8] withclient docstring --- packages/hub/src/hub.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 0bae9f0b8ac6..6c5173d0d1c4 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -473,8 +473,7 @@ export class Hub implements HubInterface { /** * Internal helper function to call a method on the top client if it exists. * - * @param method The method to call on the client. - * @param args Arguments to pass to the client function. + * @param callback Callback that gets passed client and scope from stack */ // eslint-disable-next-line @typescript-eslint/no-explicit-any private _withClient(callback: (client: Client, scope: Scope | undefined) => void): void { From 2847a14734503d729141e6fb0d43c852e599d7cc Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 1 Dec 2021 19:43:25 -0500 Subject: [PATCH 7/8] remove _withClient from minimal --- packages/minimal/src/index.ts | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index bd00e6233d9b..7325c95cb97d 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -2,7 +2,6 @@ import { getCurrentHub, Hub, Scope } from '@sentry/hub'; import { Breadcrumb, CaptureContext, - Client, CustomSamplingContext, Event, Extra, @@ -180,18 +179,6 @@ export function withScope(callback: (scope: Scope) => void): void { callOnHub('withScope', callback); } -/** - * Executes a callback on the hub that is given a client and scope if they exist. If no client - * exists, the callback will not execute. - * - * @param callback that is called with client and scope. - * @hidden - */ -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export function _withClient(callback: (client: Client, scope: Scope | undefined) => void): void { - callOnHub('_withClient', callback); -} - /** * Starts a new `Transaction` and returns it. This is the entry point to manual tracing instrumentation. * From d923d485351a2917f0c9434826b2a811e7123cf2 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 1 Dec 2021 19:46:56 -0500 Subject: [PATCH 8/8] update changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 811f659948ce..e35fad0ddb75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +- ref(hub): Remove `_invokeClient` (#4195) +- **breaking** feat(minimal): Remove `_callOnClient` (#4195) + +`_callOnClient` was a hidden API that was not meant for public use. If this breaks your set up, please write a GitHub issue describing your usage of `_callOnClient` and we can re-evaluate accordingly. + ## 6.15.0 - fix(browser): Capture stacktrace on `DOMExceptions`, if possible (#4160)