Use SyncPromise internally & Remove deprecated API & Trim Public API #1858

HazAT · 2019-01-28T21:29:33Z

This PR changes the internals of all the SDKs to be less async.
This means that instead of internally using async/await / Promise mostly everywhere, we now use our own implementation SyncPromise.

SyncPromise is as the name says sync if everything is called directly.
This has a lot of advantages when it comes to very high volume calls to our SDKs.

Right now this is marked as a major bump because of the huge internal changes.
The public facing API shouldn't change, only the API for Client changes.
Also, we are hiding the Backend from the package, to not expose SyncPromise.

If you were using the minimal package or the SDK as we advertised it in the official docs, you shouldn't notice anything.

getsentry-bot · 2019-01-28T21:33:47Z

	Warnings
⚠️	Please add a changelog entry for your changes.

	Messages
📖	✅ TSLint passed
📖	@sentry/browser gzip'ed minified size: 21.6426 kB

Generated by 🚫 dangerJS

CHANGELOG.md

packages/browser/test/backend.test.ts

kamilogorek · 2019-02-05T14:15:36Z

packages/browser/src/backend.ts

-      event = await this.eventFromMessage(ex, undefined, hint);
-      addExceptionTypeValue(event, `${ex}`);
-    }
+  public eventFromException(exception: any, hint?: SentryEventHint): SyncPromise<SentryEvent> {


I'm not fully convinced about this SyncPromise stuff tbh. It adds a level of indirection when in reality we could just use the fully procedural code here as well.

This is correct, problem is that in node this needs to be "async" meaning that we can do file io in node. The function is defined in the interface of Backend, so even though this fully sync in browser, it isn't in node.

packages/core/src/baseclient.ts

packages/utils/src/syncpromise.ts

kamilogorek · 2019-02-05T15:43:51Z

packages/utils/src/syncpromise.ts

+
+  /** JSDoc */
+  private readonly attachHandler = (handler: Handler<T, any>) => {
+    this.handlers = this.handlers.concat(handler);


This one is odd, as attaching handler always executes it anyway. What's the point of keeping track of them then?

packages/utils/src/syncpromise.ts

kamilogorek · 2019-02-05T15:47:46Z

packages/utils/test/is.test.ts

+describe('isThenable()', () => {
+  test('should work as advertised', () => {
+    expect(isThenable(Promise.resolve(true))).toEqual(true);
+    // expect(isThenable(async () => false)).toEqual(true);


It would be nice to add SyncPromise here as well.

Co-Authored-By: HazAT <[email protected]>

HazAT · 2019-02-08T13:20:26Z

Please do not delete the branch yet, we delete it when we merge 5.0.

do not delete the branch yet

…1858) * ref: Move PromiseBuffer and Error to utils package * ref: Remove all async api * ref: Remove invokeClientAsync function * feat: Finish Node SDK * feat: SyncPromise implementation * fix: Browser SDK * fix: beforeSend callback * fix: Core tests * fix: linting and tests * fix: browser npm package + add error for manual tests * meta: Remove deprecations * fix: Typedocs and public exposed functions * meta: Changelog * Apply suggestions from code review Co-Authored-By: HazAT <[email protected]> * fix: CodeReview * feat: #1871

* fix: Events created from exception shouldnt have top-level message attribute (#1831) * Use SyncPromise internally & Remove deprecated API & Trim Public API (#1858) * ref: Move PromiseBuffer and Error to utils package * ref: Remove all async api * ref: Remove invokeClientAsync function * feat: Finish Node SDK * feat: SyncPromise implementation * fix: Browser SDK * fix: beforeSend callback * fix: Core tests * fix: linting and tests * fix: browser npm package + add error for manual tests * meta: Remove deprecations * fix: Typedocs and public exposed functions * meta: Changelog * Apply suggestions from code review Co-Authored-By: HazAT <[email protected]> * fix: CodeReview * feat: #1871 * ref: Move public interfaces to types package Move addBreadcrumb from the client to the hub * fix: Tests * fix: Small type issues * ref: Remove scope listeners * fix: Add flush to interface * ref: Remove interfaces from core

…1858) * ref: Move PromiseBuffer and Error to utils package * ref: Remove all async api * ref: Remove invokeClientAsync function * feat: Finish Node SDK * feat: SyncPromise implementation * fix: Browser SDK * fix: beforeSend callback * fix: Core tests * fix: linting and tests * fix: browser npm package + add error for manual tests * meta: Remove deprecations * fix: Typedocs and public exposed functions * meta: Changelog * Apply suggestions from code review Co-Authored-By: HazAT <[email protected]> * fix: CodeReview * feat: #1871

* fix: Events created from exception shouldnt have top-level message attribute (#1831) * Use SyncPromise internally & Remove deprecated API & Trim Public API (#1858) * ref: Move PromiseBuffer and Error to utils package * ref: Remove all async api * ref: Remove invokeClientAsync function * feat: Finish Node SDK * feat: SyncPromise implementation * fix: Browser SDK * fix: beforeSend callback * fix: Core tests * fix: linting and tests * fix: browser npm package + add error for manual tests * meta: Remove deprecations * fix: Typedocs and public exposed functions * meta: Changelog * Apply suggestions from code review Co-Authored-By: HazAT <[email protected]> * fix: CodeReview * feat: #1871 * ref: Move public interfaces to types package Move addBreadcrumb from the client to the hub * fix: Tests * fix: Small type issues * ref: Remove scope listeners * fix: Add flush to interface * ref: Remove interfaces from core

…1858) * ref: Move PromiseBuffer and Error to utils package * ref: Remove all async api * ref: Remove invokeClientAsync function * feat: Finish Node SDK * feat: SyncPromise implementation * fix: Browser SDK * fix: beforeSend callback * fix: Core tests * fix: linting and tests * fix: browser npm package + add error for manual tests * meta: Remove deprecations * fix: Typedocs and public exposed functions * meta: Changelog * Apply suggestions from code review Co-Authored-By: HazAT <[email protected]> * fix: CodeReview * feat: #1871

* fix: Events created from exception shouldnt have top-level message attribute (#1831) * Use SyncPromise internally & Remove deprecated API & Trim Public API (#1858) * ref: Move PromiseBuffer and Error to utils package * ref: Remove all async api * ref: Remove invokeClientAsync function * feat: Finish Node SDK * feat: SyncPromise implementation * fix: Browser SDK * fix: beforeSend callback * fix: Core tests * fix: linting and tests * fix: browser npm package + add error for manual tests * meta: Remove deprecations * fix: Typedocs and public exposed functions * meta: Changelog * Apply suggestions from code review Co-Authored-By: HazAT <[email protected]> * fix: CodeReview * feat: #1871 * ref: Move public interfaces to types package Move addBreadcrumb from the client to the hub * fix: Tests * fix: Small type issues * ref: Remove scope listeners * fix: Add flush to interface * ref: Remove interfaces from core

…PIs (#1858) * ref: Move PromiseBuffer and Error to utils package * ref: Remove all async api * ref: Remove invokeClientAsync function * feat: Finish Node SDK * feat: SyncPromise implementation * fix: Browser SDK * fix: beforeSend callback * fix: Core tests * fix: linting and tests * fix: browser npm package + add error for manual tests * meta: Remove deprecations * fix: Typedocs and public exposed functions * meta: Changelog * Apply suggestions from code review

* fix: Events created from exception shouldnt have top-level message attribute (#1831) * Use SyncPromise internally & Remove deprecated API & Trim Public API (#1858) * ref: Move PromiseBuffer and Error to utils package * ref: Remove all async api * ref: Remove invokeClientAsync function * feat: Finish Node SDK * feat: SyncPromise implementation * fix: Browser SDK * fix: beforeSend callback * fix: Core tests * fix: linting and tests * fix: browser npm package + add error for manual tests * meta: Remove deprecations * fix: Typedocs and public exposed functions * meta: Changelog * Apply suggestions from code review * ref: Move public interfaces to types package * fix: Tests * fix: Small type issues * ref: Remove scope listeners * fix: Add flush to interface * ref: Remove interfaces from core

…PIs (#1858) * ref: Move PromiseBuffer and Error to utils package * ref: Remove all async api * ref: Remove invokeClientAsync function * feat: Finish Node SDK * feat: SyncPromise implementation * fix: Browser SDK * fix: beforeSend callback * fix: Core tests * fix: linting and tests * fix: browser npm package + add error for manual tests * meta: Remove deprecations * fix: Typedocs and public exposed functions * meta: Changelog * Apply suggestions from code review

* fix: Events created from exception shouldnt have top-level message attribute (#1831) * Use SyncPromise internally & Remove deprecated API & Trim Public API (#1858) * ref: Move PromiseBuffer and Error to utils package * ref: Remove all async api * ref: Remove invokeClientAsync function * feat: Finish Node SDK * feat: SyncPromise implementation * fix: Browser SDK * fix: beforeSend callback * fix: Core tests * fix: linting and tests * fix: browser npm package + add error for manual tests * meta: Remove deprecations * fix: Typedocs and public exposed functions * meta: Changelog * Apply suggestions from code review * ref: Move public interfaces to types package * fix: Tests * fix: Small type issues * ref: Remove scope listeners * fix: Add flush to interface * ref: Remove interfaces from core

…PIs (#1858) * ref: Move PromiseBuffer and Error to utils package * ref: Remove all async api * ref: Remove invokeClientAsync function * feat: Finish Node SDK * feat: SyncPromise implementation * fix: Browser SDK * fix: beforeSend callback * fix: Core tests * fix: linting and tests * fix: browser npm package + add error for manual tests * meta: Remove deprecations * fix: Typedocs and public exposed functions * meta: Changelog * Apply suggestions from code review

* fix: Events created from exception shouldnt have top-level message attribute (#1831) * Use SyncPromise internally & Remove deprecated API & Trim Public API (#1858) * ref: Move PromiseBuffer and Error to utils package * ref: Remove all async api * ref: Remove invokeClientAsync function * feat: Finish Node SDK * feat: SyncPromise implementation * fix: Browser SDK * fix: beforeSend callback * fix: Core tests * fix: linting and tests * fix: browser npm package + add error for manual tests * meta: Remove deprecations * fix: Typedocs and public exposed functions * meta: Changelog * Apply suggestions from code review * ref: Move public interfaces to types package * fix: Tests * fix: Small type issues * ref: Remove scope listeners * fix: Add flush to interface * ref: Remove interfaces from core

HazAT added 8 commits January 21, 2019 12:57

ref: Move PromiseBuffer and Error to utils package

200e046

ref: Remove all async api

5a82777

ref: Remove invokeClientAsync function

8cdee67

feat: Finish Node SDK

3ada698

feat: SyncPromise implementation

afcdb4c

fix: Browser SDK

68ff201

fix: beforeSend callback

cfcf366

fix: Core tests

53b4744

HazAT added the Release: v5 label Jan 28, 2019

HazAT self-assigned this Jan 28, 2019

HazAT requested a review from kamilogorek as a code owner January 28, 2019 21:29

HazAT added this to the 5.0.0 milestone Jan 28, 2019

HazAT added Meta: Breaking and removed Release: v5 labels Jan 28, 2019

HazAT added 3 commits January 28, 2019 23:13

fix: linting and tests

70dafe3

fix: browser npm package + add error for manual tests

cb00da2

meta: Remove deprecations

2b84aed

HazAT changed the title ~~[WIP] 5.0.0~~ [WIP] Use SyncPromise internally Jan 31, 2019

HazAT changed the base branch from master to major/5 January 31, 2019 11:09

kamilogorek force-pushed the major/5 branch from 3ce415e to 4979bd8 Compare January 31, 2019 12:49

HazAT added 2 commits February 4, 2019 12:34

Merge branch 'major/5' into ref/make-sync

df0c3d7

fix: Typedocs and public exposed functions

8ea2ec7

HazAT changed the title ~~[WIP] Use SyncPromise internally~~ [WIP] Use SyncPromise internally & Remove deprecated API & Trim Public API Feb 4, 2019

meta: Changelog

0b95f34

kamilogorek suggested changes Feb 5, 2019

View reviewed changes

kamilogorek and others added 2 commits February 6, 2019 09:06

Apply suggestions from code review

5ca41a8

Co-Authored-By: HazAT <[email protected]>

fix: CodeReview

7875c0b

HazAT changed the title ~~[WIP] Use SyncPromise internally & Remove deprecated API & Trim Public API~~ Use SyncPromise internally & Remove deprecated API & Trim Public API Feb 6, 2019

feat: #1871

14ffee1

kamilogorek approved these changes Feb 8, 2019

View reviewed changes

HazAT merged commit 459fdd8 into major/5 Feb 8, 2019

HazAT mentioned this pull request Feb 8, 2019

[WIP] Major Version 5 #1861

Closed

8 tasks

HazAT mentioned this pull request Feb 21, 2019

v5.0.0 #1919

Merged

14 tasks

HazAT deleted the ref/make-sync branch March 19, 2019 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use SyncPromise internally & Remove deprecated API & Trim Public API #1858

Use SyncPromise internally & Remove deprecated API & Trim Public API #1858

HazAT commented Jan 28, 2019

getsentry-bot commented Jan 28, 2019 •

edited

Loading

kamilogorek Feb 5, 2019

HazAT Feb 6, 2019

kamilogorek Feb 5, 2019

kamilogorek Feb 5, 2019

HazAT commented Feb 8, 2019 •

edited by kamilogorek

Loading

Use SyncPromise internally & Remove deprecated API & Trim Public API #1858

Use SyncPromise internally & Remove deprecated API & Trim Public API #1858

Conversation

HazAT commented Jan 28, 2019

getsentry-bot commented Jan 28, 2019 • edited Loading

kamilogorek Feb 5, 2019

Choose a reason for hiding this comment

HazAT Feb 6, 2019

Choose a reason for hiding this comment

kamilogorek Feb 5, 2019

Choose a reason for hiding this comment

kamilogorek Feb 5, 2019

Choose a reason for hiding this comment

HazAT commented Feb 8, 2019 • edited by kamilogorek Loading

do not delete the branch yet

getsentry-bot commented Jan 28, 2019 •

edited

Loading

HazAT commented Feb 8, 2019 •

edited by kamilogorek

Loading