Code review patches

Author: kamilogorek

packages/browser/src/sdk.ts

@@ -181,31 +181,33 @@ export function wrap(fn: (...args: any) => any): any {
 function startSessionTracking(): void {
   const window = getGlobalObject<Window>();
   const hub = getCurrentHub();
+
+  /**
+   * We should be using `Promise.all([windowLoaded, firstContentfulPaint])` here,
+   * but, as always, it's not available in the IE10-11. Thanks IE.
+   */
+  let loadResolved = document.readyState === 'complete';
   let fcpResolved = false;
-  let loadResolved = false;
   const possiblyEndSession = (): void => {
     if (fcpResolved && loadResolved) {
       hub.endSession();
     }
   };
+  const resolveWindowLoaded = (): void => {
+    loadResolved = true;
+    possiblyEndSession();
+    window.removeEventListener('load', resolveWindowLoaded);
+  };
 
   hub.startSession();
 
-  window.addEventListener('load', () => {
-    loadResolved = true;
-    possiblyEndSession();
-  });
+  if (!loadResolved) {
+    // IE doesn't support `{ once: true }` for event listeners, so we have to manually
+    // attach and then detach it once completed.
+    window.addEventListener('load', resolveWindowLoaded);
+  }
 
   try {
-    let firstHiddenTime = document.visibilityState === 'hidden' ? 0 : Infinity;
-    document.addEventListener(
-      'visibilitychange',
-      event => {
-        firstHiddenTime = Math.min(firstHiddenTime, event.timeStamp);
-      },
-      { once: true },
-    );
-
     const po = new PerformanceObserver((entryList, po) => {
       entryList.getEntries().forEach(entry => {
         if (entry.name === 'first-contentful-paint' && entry.startTime < firstHiddenTime) {
@@ -216,6 +218,17 @@ function startSessionTracking(): void {
       });
     });
 
+    // There's no need to even attach this listener if `PerformanceObserver` constructor will fail,
+    // so we do it below here.
+    let firstHiddenTime = document.visibilityState === 'hidden' ? 0 : Infinity;
+    document.addEventListener(
+      'visibilitychange',
+      event => {
+        firstHiddenTime = Math.min(firstHiddenTime, event.timeStamp);
+      },
+      { once: true },
+    );
+
     po.observe({
       type: 'paint',
       buffered: true,

packages/browser/src/transports/fetch.ts

@@ -24,12 +24,13 @@ export class FetchTransport extends BaseTransport {
 
   /**
    * @param sentryRequest Prepared SentryRequest to be delivered
-   * @param event original payload used to create SentryRequest
+   * @param originalPayload Original payload used to create SentryRequest
    */
-  private _sendRequest(sentryRequest: SentryRequest, event: Event | Session): PromiseLike<Response> {
+  private _sendRequest(sentryRequest: SentryRequest, originalPayload: Event | Session): PromiseLike<Response> {
     if (this._isRateLimited(sentryRequest.type)) {
       return Promise.reject({
-        event,
+        event: originalPayload,
+        type: sentryRequest.type,
         reason: `Transport locked till ${this._disabledUntil(sentryRequest.type)} due to too many requests.`,
         status: 429,
       });

packages/browser/src/transports/xhr.ts

@@ -22,12 +22,13 @@ export class XHRTransport extends BaseTransport {
 
   /**
    * @param sentryRequest Prepared SentryRequest to be delivered
-   * @param event original payload used to create SentryRequest
+   * @param originalPayload Original payload used to create SentryRequest
    */
-  private _sendRequest(sentryRequest: SentryRequest, event: Event | Session): PromiseLike<Response> {
+  private _sendRequest(sentryRequest: SentryRequest, originalPayload: Event | Session): PromiseLike<Response> {
     if (this._isRateLimited(sentryRequest.type)) {
       return Promise.reject({
-        event,
+        event: originalPayload,
+        type: sentryRequest.type,
         reason: `Transport locked till ${this._disabledUntil(sentryRequest.type)} due to too many requests.`,
         status: 429,
       });

packages/core/src/basebackend.ts

@@ -101,7 +101,7 @@ export abstract class BaseBackend<O extends Options> implements Backend {
    */
   public sendSession(session: Session): void {
     if (!this._transport.sendSession) {
-      logger.warn('sendSession not implemented for a transport used');
+      logger.warn("Dropping session because custom transport doesn't implement sendSession");
       return;
     }
 

packages/core/src/baseclient.ts

@@ -494,16 +494,14 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
 
           let finalEvent: Event | null = prepared;
 
-          const isInternalException =
-            hint && hint.data && (hint.data as { [key: string]: unknown }).__sentry__ === true;
-          // We skip beforeSend in case of transactions
-
-          if (isInternalException || !beforeSend || isTransaction) {
-            if (!isTransaction) {
-              const session = scope && scope.getSession();
-              if (session) {
-                this._updateSessionFromEvent(session, finalEvent);
-              }
+          const isInternalException = hint && hint.data && (hint.data as { __sentry__: boolean }).__sentry__ === true;
+          const shouldSkipBeforeSend = isInternalException || isTransaction || !beforeSend;
+          const session = scope && scope.getSession();
+
+          if (shouldSkipBeforeSend) {
+            // If this is a generic error event and we have a session available, update it.
+            if (!isTransaction && session) {
+              this._updateSessionFromEvent(session, finalEvent);
             }
 
             this._sendEvent(finalEvent);
@@ -525,7 +523,6 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
               return;
             }
 
-            const session = scope && scope.getSession();
             if (session) {
               this._updateSessionFromEvent(session, finalEvent);
             }

packages/core/src/request.ts

@@ -1,12 +1,11 @@
 import { Event, SentryRequest, Session } from '@sentry/types';
-import { timestampWithMs } from '@sentry/utils';
 
 import { API } from './api';
 
 /** Creates a SentryRequest from an event. */
 export function sessionToSentryRequest(session: Session, api: API): SentryRequest {
   const envelopeHeaders = JSON.stringify({
-    sent_at: new Date(timestampWithMs() * 1000).toISOString(),
+    sent_at: new Date().toISOString(),
   });
   const itemHeaders = JSON.stringify({
     type: 'session',

packages/hub/src/session.ts

@@ -80,9 +80,6 @@ export class Session implements SessionInterface {
       this.update({ status });
     } else if (this.status === SessionStatus.Ok) {
       this.update({ status: SessionStatus.Exited });
-    } else {
-      // Calling `update` alone updates session.timestamp to the current time and session.duration.
-      this.update();
     }
   }
 

packages/tracing/src/browser/browsertracing.ts

@@ -190,7 +190,6 @@ export class BrowserTracing implements Integration {
 
     // eslint-disable-next-line @typescript-eslint/unbound-method
     const { beforeNavigate, idleTimeout, maxTransactionDuration } = this.options;
-    const hub = this._getCurrentHub();
     const parentContextFromHeader = context.op === 'pageload' ? getHeaderContext() : undefined;
 
     const expandedContext = {
@@ -208,6 +207,7 @@ export class BrowserTracing implements Integration {
       logger.log(`[Tracing] Will not send ${finalContext.op} transaction because of beforeNavigate.`);
     }
 
+    const hub = this._getCurrentHub();
     const idleTransaction = startIdleTransaction(hub, finalContext, idleTimeout, true);
     logger.log(`[Tracing] Starting ${finalContext.op} transaction on scope`);
     idleTransaction.registerBeforeFinishCallback((transaction, endTimestamp) => {

packages/types/src/hub.ts

@@ -208,7 +208,7 @@ export interface Hub {
    * To finish a `session`, it has to be passed directly to `client.captureSession`, which is done automatically
    * when using `hub.endSession()` for the session currently stored on the scope.
    *
-   * When there's already an existing session on the scope, it'll be autmatically ended.
+   * When there's already an existing session on the scope, it'll be automatically ended.
    *
    * @param context Optional properties of the new `Session`.
    *

packages/types/src/request.ts

@@ -6,11 +6,6 @@ export interface SentryRequest {
   body: string;
   type: SentryRequestType;
   url: string;
-  // headers would contain auth & content-type headers for @sentry/node, but
-  // since @sentry/browser avoids custom headers to prevent CORS preflight
-  // requests, we can use the same approach for @sentry/browser and @sentry/node
-  // for simplicity -- no headers involved.
-  // headers: { [key: string]: string };
 }
 
 /** Request data included in an event as sent to Sentry */

packages/types/src/response.ts

@@ -1,9 +1,11 @@
-import { Event } from './event';
+import { Event, EventType } from './event';
+import { Session } from './session';
 import { Status } from './status';
 
 /** JSDoc */
 export interface Response {
   status: Status;
-  event?: Event;
+  event?: Event | Session;
+  type?: EventType;
   reason?: string;
 }