Skip to content

Commit 7875c0b

Browse files
HazATHazAT
committed
fix: CodeReview
1 parent 5ca41a8 commit 7875c0b

File tree

8 files changed

+259
-424
lines changed

8 files changed

+259
-424
lines changed

‎packages/browser/src/backend.ts

Lines changed: 48 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -75,57 +75,57 @@ export class BrowserBackend extends BaseBackend<BrowserOptions> {
7575
* @inheritDoc
7676
*/
7777
public eventFromException(exception: any, hint?: SentryEventHint): SyncPromise<SentryEvent> {
78-
return new SyncPromise<SentryEvent>(resolve => {
79-
let event: SentryEvent;
80-
81-
if (isErrorEvent(exception as ErrorEvent) && (exception as ErrorEvent).error) {
82-
// If it is an ErrorEvent with `error` property, extract it to get actual Error
83-
const ex = exception as ErrorEvent;
84-
exception = ex.error; // tslint:disable-line:no-parameter-reassignment
85-
event = eventFromStacktrace(computeStackTrace(exception as Error));
86-
resolve(this.buildEvent(event));
87-
} else if (isDOMError(exception as DOMError) || isDOMException(exception as DOMException)) {
88-
// If it is a DOMError or DOMException (which are legacy APIs, but still supported in some browsers)
89-
// then we just extract the name and message, as they don't provide anything else
90-
// https://developer.mozilla.org/en-US/docs/Web/API/DOMError
91-
// https://developer.mozilla.org/en-US/docs/Web/API/DOMException
92-
const ex = exception as DOMException;
93-
const name = ex.name || (isDOMError(ex) ? 'DOMError' : 'DOMException');
94-
const message = ex.message ? `${name}: ${ex.message}` : name;
95-
96-
this.eventFromMessage(message, undefined, hint).then(messageEvent => {
97-
addExceptionTypeValue(messageEvent, message);
98-
resolve(this.buildEvent(messageEvent));
99-
});
100-
} else if (isError(exception as Error)) {
101-
// we have a real Error object, do nothing
102-
event = eventFromStacktrace(computeStackTrace(exception as Error));
103-
resolve(this.buildEvent(event));
104-
} else if (isPlainObject(exception as {}) && hint && hint.syntheticException) {
105-
// If it is plain Object, serialize it manually and extract options
106-
// This will allow us to group events based on top-level keys
107-
// which is much better than creating new group when any key/value change
108-
const ex = exception as {};
109-
event = eventFromPlainObject(ex, hint.syntheticException);
110-
addExceptionTypeValue(event, 'Custom Object');
111-
resolve(this.buildEvent(event));
112-
} else {
113-
// If none of previous checks were valid, then it means that
114-
// it's not a DOMError/DOMException
115-
// it's not a plain Object
116-
// it's not a valid ErrorEvent (one with an error property)
117-
// it's not an Error
118-
// So bail out and capture it as a simple message:
119-
const ex = exception as string;
120-
this.eventFromMessage(ex, undefined, hint).then(messageEvent => {
121-
addExceptionTypeValue(messageEvent, `${ex}`);
122-
resolve(this.buildEvent(messageEvent));
123-
});
124-
}
78+
let event: SentryEvent;
79+
80+
if (isErrorEvent(exception as ErrorEvent) && (exception as ErrorEvent).error) {
81+
// If it is an ErrorEvent with `error` property, extract it to get actual Error
82+
const errorEvent = exception as ErrorEvent;
83+
exception = errorEvent.error; // tslint:disable-line:no-parameter-reassignment
84+
event = eventFromStacktrace(computeStackTrace(exception as Error));
85+
return SyncPromise.resolve(this.buildEvent(event));
86+
} else if (isDOMError(exception as DOMError) || isDOMException(exception as DOMException)) {
87+
// If it is a DOMError or DOMException (which are legacy APIs, but still supported in some browsers)
88+
// then we just extract the name and message, as they don't provide anything else
89+
// https://developer.mozilla.org/en-US/docs/Web/API/DOMError
90+
// https://developer.mozilla.org/en-US/docs/Web/API/DOMException
91+
const domException = exception as DOMException;
92+
const name = domException.name || (isDOMError(domException) ? 'DOMError' : 'DOMException');
93+
const message = domException.message ? `${name}: ${domException.message}` : name;
94+
95+
return this.eventFromMessage(message, undefined, hint).then(messageEvent => {
96+
addExceptionTypeValue(messageEvent, message);
97+
return SyncPromise.resolve(this.buildEvent(messageEvent));
98+
});
99+
} else if (isError(exception as Error)) {
100+
// we have a real Error object, do nothing
101+
event = eventFromStacktrace(computeStackTrace(exception as Error));
102+
return SyncPromise.resolve(this.buildEvent(event));
103+
} else if (isPlainObject(exception as {}) && hint && hint.syntheticException) {
104+
// If it is plain Object, serialize it manually and extract options
105+
// This will allow us to group events based on top-level keys
106+
// which is much better than creating new group when any key/value change
107+
const objectException = exception as {};
108+
event = eventFromPlainObject(objectException, hint.syntheticException);
109+
addExceptionTypeValue(event, 'Custom Object');
110+
return SyncPromise.resolve(this.buildEvent(event));
111+
}
112+
113+
// If none of previous checks were valid, then it means that
114+
// it's not a DOMError/DOMException
115+
// it's not a plain Object
116+
// it's not a valid ErrorEvent (one with an error property)
117+
// it's not an Error
118+
// So bail out and capture it as a simple message:
119+
const stringException = exception as string;
120+
return this.eventFromMessage(stringException, undefined, hint).then(messageEvent => {
121+
addExceptionTypeValue(messageEvent, `${stringException}`);
122+
return SyncPromise.resolve(this.buildEvent(messageEvent));
125123
});
126124
}
127125

128-
/** JSDOC */
126+
/**
127+
* This is an internal helper function that creates an event.
128+
*/
129129
private buildEvent(event: SentryEvent, hint?: SentryEventHint): SentryEvent {
130130
return {
131131
...event,

‎packages/browser/test/backend.test.ts

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,13 @@
11
import { expect } from 'chai';
22
import { BrowserBackend } from '../src/backend';
33

4-
const dsn = 'https://[email protected]/42';
5-
const testEvent = {
6-
event_id: '1337',
7-
message: 'Pickle Rick',
8-
user: {
9-
username: 'Morty',
10-
},
11-
};
12-
134
let backend: BrowserBackend;
145

156
describe('BrowserBackend', () => {
167
describe('sendEvent()', () => {
17-
it('should throw when no Dsn is provided', async () => {
18-
backend = new BrowserBackend({ dsn });
19-
20-
try {
21-
backend.sendEvent(testEvent);
22-
} catch (e) {
23-
expect(e.message).equal('Cannot sendEvent without a valid Dsn');
24-
}
8+
it('should use NoopTransport', async () => {
9+
backend = new BrowserBackend({});
10+
expect(backend.getTransport().constructor.name).to.equal('NoopTransport');
2511
});
2612
});
2713
});

‎packages/core/src/baseclient.ts

Lines changed: 77 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -135,10 +135,12 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
135135

136136
this.getBackend()
137137
.eventFromException(exception, hint)
138-
.then(event => {
139-
this.processEvent(event, hint, scope).then(finalEvent => {
140-
eventId = (finalEvent && finalEvent.event_id) || undefined;
141-
});
138+
.then(event => this.processEvent(event, hint, scope))
139+
.then(finalEvent => {
140+
eventId = finalEvent.event_id;
141+
})
142+
.catch(reason => {
143+
logger.log(reason);
142144
});
143145

144146
return eventId;
@@ -154,11 +156,14 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
154156
? this.getBackend().eventFromMessage(`${message}`, level, hint)
155157
: this.getBackend().eventFromException(message, hint);
156158

157-
promisedEvent.then(event => {
158-
this.processEvent(event, hint, scope).then(finalEvent => {
159-
eventId = (finalEvent && finalEvent.event_id) || undefined;
159+
promisedEvent
160+
.then(event => this.processEvent(event, hint, scope))
161+
.then(finalEvent => {
162+
eventId = finalEvent.event_id;
163+
})
164+
.catch(reason => {
165+
logger.log(reason);
160166
});
161-
});
162167

163168
return eventId;
164169
}
@@ -168,9 +173,13 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
168173
*/
169174
public captureEvent(event: SentryEvent, hint?: SentryEventHint, scope?: Scope): string | undefined {
170175
let eventId: string | undefined = hint && hint.event_id;
171-
this.processEvent(event, hint, scope).then(finalEvent => {
172-
eventId = (finalEvent && finalEvent.event_id) || undefined;
173-
});
176+
this.processEvent(event, hint, scope)
177+
.then(finalEvent => {
178+
eventId = finalEvent.event_id;
179+
})
180+
.catch(reason => {
181+
logger.log(reason);
182+
});
174183
return eventId;
175184
}
176185

@@ -270,13 +279,17 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
270279
prepared.event_id = uuid4();
271280
}
272281

282+
// We prepare the result here with a resolved SentryEvent.
283+
let result = SyncPromise.resolve<SentryEvent | null>(prepared);
284+
273285
// This should be the last thing called, since we want that
274286
// {@link Hub.addEventProcessor} gets the finished prepared event.
275287
if (scope) {
276-
return scope.applyToEvent(prepared, hint, Math.min(maxBreadcrumbs, MAX_BREADCRUMBS));
288+
// In case we have a hub we reassign it.
289+
result = scope.applyToEvent(prepared, hint, Math.min(maxBreadcrumbs, MAX_BREADCRUMBS));
277290
}
278291

279-
return SyncPromise.resolve(prepared as SentryEvent | null);
292+
return result;
280293
}
281294

282295
/**
@@ -286,80 +299,57 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
286299
* platform specific meta data (such as the User's IP address) must be added
287300
* by the SDK implementor.
288301
*
289-
* The returned event status offers clues to whether the event was sent to
290-
* Sentry and accepted there. If the {@link Options.shouldSend} hook returns
291-
* `false`, the status will be {@link SendStatus.Skipped}. If the rate limit
292-
* was exceeded, the status will be {@link SendStatus.RateLimit}.
293302
*
294303
* @param event The event to send to Sentry.
295-
* @param send A function to actually send the event.
296-
* @param scope A scope containing event metadata.
297304
* @param hint May contain additional informartion about the original exception.
298-
* @returns A SyncPromise that resolves with the event.
305+
* @param scope A scope containing event metadata.
306+
* @returns A SyncPromise that resolves with the event or rejects in case event was/will not be send.
299307
*/
300-
protected processEvent(event: SentryEvent, hint?: SentryEventHint, scope?: Scope): SyncPromise<SentryEvent | null> {
308+
protected processEvent(event: SentryEvent, hint?: SentryEventHint, scope?: Scope): SyncPromise<SentryEvent> {
301309
const { beforeSend, sampleRate } = this.getOptions();
302310

303311
if (!this.isEnabled()) {
304-
logger.log('SDK not enabled, will not send event.');
305-
return SyncPromise.resolve();
312+
return SyncPromise.reject('SDK not enabled, will not send event.');
306313
}
307314

308315
// 1.0 === 100% events are sent
309316
// 0.0 === 0% events are sent
310317
if (typeof sampleRate === 'number' && Math.random() > sampleRate) {
311-
logger.log('This event has been sampled, will not send event.');
312-
return SyncPromise.resolve();
318+
return SyncPromise.reject('This event has been sampled, will not send event.');
313319
}
314320

315-
return new SyncPromise(resolve => {
321+
return new SyncPromise((resolve, reject) => {
316322
this.prepareEvent(event, scope, hint).then(prepared => {
317323
if (prepared === null) {
318-
logger.log('An event processor returned null, will not send event.');
319-
resolve(null);
324+
reject('An event processor returned null, will not send event.');
320325
return;
321326
}
322327

323328
let finalEvent: SentryEvent | null = prepared;
324329

325330
try {
326331
const isInternalException = hint && hint.data && (hint.data as { [key: string]: any }).__sentry__ === true;
327-
if (!isInternalException && beforeSend) {
328-
const beforeSendResult = beforeSend(prepared, hint);
329-
if ((typeof beforeSendResult as any) === 'undefined') {
330-
logger.error('`beforeSend` method has to return `null` or a valid event.');
331-
} else if (isThenable(beforeSendResult)) {
332-
(beforeSendResult as Promise<SentryEvent | null>)
333-
.then(processedEvent => {
334-
finalEvent = processedEvent;
335-
336-
if (finalEvent === null) {
337-
logger.log('`beforeSend` returned `null`, will not send event.');
338-
resolve(null);
339-
return;
340-
}
341-
342-
// From here on we are really async
343-
this.getBackend().sendEvent(finalEvent);
344-
resolve(finalEvent);
345-
})
346-
.catch(e => {
347-
logger.error(`beforeSend rejected with ${e}`);
348-
});
349-
} else {
350-
finalEvent = beforeSendResult as SentryEvent | null;
351-
352-
if (finalEvent === null) {
353-
logger.log('`beforeSend` returned `null`, will not send event.');
354-
resolve(null);
355-
return;
356-
}
357-
358-
// From here on we are really async
359-
this.getBackend().sendEvent(finalEvent);
360-
resolve(finalEvent);
361-
}
332+
if (isInternalException || !beforeSend) {
333+
this.getBackend().sendEvent(finalEvent);
334+
resolve(finalEvent);
335+
return;
336+
}
337+
338+
const beforeSendResult = beforeSend(prepared, hint);
339+
if ((typeof beforeSendResult as any) === 'undefined') {
340+
logger.error('`beforeSend` method has to return `null` or a valid event.');
341+
} else if (isThenable(beforeSendResult)) {
342+
this.handleAsyncBeforeSend(beforeSendResult as Promise<SentryEvent | null>, resolve, reject);
362343
} else {
344+
finalEvent = beforeSendResult as SentryEvent | null;
345+
346+
if (finalEvent === null) {
347+
logger.log('`beforeSend` returned `null`, will not send event.');
348+
resolve(null);
349+
return;
350+
}
351+
352+
// From here on we are really async
363353
this.getBackend().sendEvent(finalEvent);
364354
resolve(finalEvent);
365355
}
@@ -370,14 +360,35 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
370360
},
371361
originalException: exception as Error,
372362
});
373-
logger.error('`beforeSend` throw an error, will not send event.');
374-
resolve(null);
375-
return;
363+
reject('`beforeSend` throw an error, will not send event.');
376364
}
377365
});
378366
});
379367
}
380368

369+
/**
370+
* Resolves before send Promise and calls resolve/reject on parent SyncPromise.
371+
*/
372+
private handleAsyncBeforeSend(
373+
beforeSend: Promise<SentryEvent | null>,
374+
resolve: (event: SentryEvent) => void,
375+
reject: (reason: string) => void,
376+
): void {
377+
beforeSend
378+
.then(processedEvent => {
379+
if (processedEvent === null) {
380+
reject('`beforeSend` returned `null`, will not send event.');
381+
return;
382+
}
383+
// From here on we are really async
384+
this.getBackend().sendEvent(processedEvent);
385+
resolve(processedEvent);
386+
})
387+
.catch(e => {
388+
reject(`beforeSend rejected with ${e}`);
389+
});
390+
}
391+
381392
/**
382393
* @inheritDoc
383394
*/

‎packages/hub/src/scope.ts

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -87,23 +87,15 @@ export class Scope {
8787
if (event === null || !isFunction(processor)) {
8888
resolve(event);
8989
} else {
90-
try {
91-
const result = processor({ ...event }, hint) as SentryEvent | null;
92-
if (isThenable(result)) {
93-
(result as Promise<SentryEvent | null>)
94-
.then((final: SentryEvent | null) => {
95-
this.notifyEventProcessors(processors, final, hint, index + 1)
96-
.then(resolve)
97-
.catch(reject);
98-
})
99-
.catch(reject);
100-
} else {
101-
this.notifyEventProcessors(processors, result, hint, index + 1)
102-
.then(resolve)
103-
.catch(reject);
104-
}
105-
} catch (e) {
106-
reject(e);
90+
const result = processor({ ...event }, hint) as SentryEvent | null;
91+
if (isThenable(result)) {
92+
(result as Promise<SentryEvent | null>)
93+
.then(final => this.notifyEventProcessors(processors, final, hint, index + 1).then(resolve))
94+
.catch(reject);
95+
} else {
96+
this.notifyEventProcessors(processors, result, hint, index + 1)
97+
.then(resolve)
98+
.catch(reject);
10799
}
108100
}
109101
});

0 commit comments

Comments
 (0)