ref: Avoid optional chaining & add eslint rule (#6777)

As this is transpiled to a rather verbose form.

Co-authored-by: Lukas Stracke <[email protected]>

Author: mydea

packages/browser/src/client.ts

@@ -96,7 +96,9 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
     const breadcrumbIntegration = this.getIntegrationById(BREADCRUMB_INTEGRATION_ID) as Breadcrumbs | undefined;
     // We check for definedness of `addSentryBreadcrumb` in case users provided their own integration with id
     // "Breadcrumbs" that does not have this function.
-    breadcrumbIntegration?.addSentryBreadcrumb?.(event);
+    if (breadcrumbIntegration && breadcrumbIntegration.addSentryBreadcrumb) {
+      breadcrumbIntegration.addSentryBreadcrumb(event);
+    }
 
     super.sendEvent(event, hint);
   }

packages/eslint-config-sdk/src/index.js

@@ -143,8 +143,9 @@ module.exports = {
           },
         ],
 
-        // We want to prevent async await usage in our files to prevent uncessary bundle size. Turned off in tests.
+        // We want to prevent async await & optional chaining usage in our files to prevent uncessary bundle size. Turned off in tests.
         '@sentry-internal/sdk/no-async-await': 'error',
+        '@sentry-internal/sdk/no-optional-chaining': 'error',
 
         // JSDOC comments are required for classes and methods. As we have a public facing codebase, documentation,
         // even if it may seems excessive at times, is important to emphasize. Turned off in tests.
@@ -178,6 +179,7 @@ module.exports = {
         '@typescript-eslint/no-non-null-assertion': 'off',
         '@typescript-eslint/no-empty-function': 'off',
         '@sentry-internal/sdk/no-async-await': 'off',
+        '@sentry-internal/sdk/no-optional-chaining': 'off',
       },
     },
     {

packages/eslint-plugin-sdk/src/index.js

@@ -11,6 +11,7 @@
 module.exports = {
   rules: {
     'no-async-await': require('./rules/no-async-await'),
+    'no-optional-chaining': require('./rules/no-optional-chaining'),
     'no-eq-empty': require('./rules/no-eq-empty'),
   },
 };

packages/eslint-plugin-sdk/src/rules/no-optional-chaining.js

@@ -0,0 +1,61 @@
+/**
+ * @fileoverview Rule to disallow using optional chaining - because this is transpiled into verbose code.
+ * @author Francesco Novy
+ *
+ * Based on https://github.com/facebook/lexical/pull/3233
+ */
+'use strict';
+
+// ------------------------------------------------------------------------------
+// Rule Definition
+// ------------------------------------------------------------------------------
+
+/** @type {import('eslint').Rule.RuleModule} */
+module.exports = {
+  meta: {
+    type: 'problem',
+    docs: {
+      description: 'disallow usage of optional chaining',
+      category: 'Best Practices',
+      recommended: true,
+    },
+    messages: {
+      forbidden: 'Avoid using optional chaining',
+    },
+    fixable: null,
+    schema: [],
+  },
+
+  create(context) {
+    const sourceCode = context.getSourceCode();
+
+    /**
+     * Checks if the given token is a `?.` token or not.
+     * @param {Token} token The token to check.
+     * @returns {boolean} `true` if the token is a `?.` token.
+     */
+    function isQuestionDotToken(token) {
+      return (
+        token.value === '?.' &&
+        (token.type === 'Punctuator' || // espree has been parsed well.
+          // [email protected] doesn't parse "?." tokens well. Therefore, get the string from the source code and check it.
+          sourceCode.getText(token) === '?.')
+      );
+    }
+
+    return {
+      'CallExpression[optional=true]'(node) {
+        context.report({
+          messageId: 'forbidden',
+          node: sourceCode.getTokenAfter(node.callee, isQuestionDotToken),
+        });
+      },
+      'MemberExpression[optional=true]'(node) {
+        context.report({
+          messageId: 'forbidden',
+          node: sourceCode.getTokenAfter(node.object, isQuestionDotToken),
+        });
+      },
+    };
+  },
+};

packages/nextjs/.eslintrc.js

@@ -10,6 +10,7 @@ module.exports = {
   extends: ['../../.eslintrc.js'],
   rules: {
     '@sentry-internal/sdk/no-async-await': 'off',
+    '@sentry-internal/sdk/no-optional-chaining': 'off',
   },
   overrides: [
     {

packages/node/.eslintrc.js

@@ -5,5 +5,6 @@ module.exports = {
   extends: ['../../.eslintrc.js'],
   rules: {
     '@sentry-internal/sdk/no-async-await': 'off',
+    '@sentry-internal/sdk/no-optional-chaining': 'off',
   },
 };

packages/opentelemetry-node/.eslintrc.js

@@ -5,5 +5,6 @@ module.exports = {
   extends: ['../../.eslintrc.js'],
   rules: {
     '@sentry-internal/sdk/no-async-await': 'off',
+    '@sentry-internal/sdk/no-optional-chaining': 'off',
   },
 };

packages/react/src/reactrouterv6.tsx

@@ -226,7 +226,7 @@ export function wrapUseRoutes(origUseRoutes: UseRoutes): UseRoutes {
 
       // A value with stable identity to either pick `locationArg` if available or `location` if not
       const stableLocationParam =
-        typeof locationArg === 'string' || locationArg?.pathname !== undefined
+        typeof locationArg === 'string' || (locationArg && locationArg.pathname)
           ? (locationArg as { pathname: string })
           : location;
 

packages/remix/.eslintrc.js

@@ -10,6 +10,7 @@ module.exports = {
   extends: ['../../.eslintrc.js'],
   rules: {
     '@sentry-internal/sdk/no-async-await': 'off',
+    '@sentry-internal/sdk/no-optional-chaining': 'off',
   },
   overrides: [
     {

packages/replay/src/coreHandlers/handleGlobalEvent.ts

@@ -21,15 +21,15 @@ export function handleGlobalEventListener(replay: ReplayContainer): (event: Even
 
     // Unless `captureExceptions` is enabled, we want to ignore errors coming from rrweb
     // As there can be a bunch of stuff going wrong in internals there, that we don't want to bubble up to users
-    if (isRrwebError(event) && !replay.getOptions()._experiments?.captureExceptions) {
+    if (isRrwebError(event) && !replay.getOptions()._experiments.captureExceptions) {
       __DEBUG_BUILD__ && logger.log('[Replay] Ignoring error from rrweb internals', event);
       return null;
     }
 
     // Only tag transactions with replayId if not waiting for an error
     // @ts-ignore private
     if (!event.type || replay.recordingMode === 'session') {
-      event.tags = { ...event.tags, replayId: replay.session?.id };
+      event.tags = { ...event.tags, replayId: replay.getSessionId() };
     }
 
     // Collect traceIds in _context regardless of `recordingMode` - if it's true,
@@ -44,12 +44,10 @@ export function handleGlobalEventListener(replay: ReplayContainer): (event: Even
       replay.getContext().errorIds.add(event.event_id as string);
     }
 
-    const exc = event.exception?.values?.[0];
-    if (__DEBUG_BUILD__ && replay.getOptions()._experiments?.traceInternals) {
+    if (__DEBUG_BUILD__ && replay.getOptions()._experiments.traceInternals) {
+      const exc = getEventExceptionValues(event);
       addInternalBreadcrumb({
-        message: `Tagging event (${event.event_id}) - ${event.message} - ${exc?.type || 'Unknown'}: ${
-          exc?.value || 'n/a'
-        }`,
+        message: `Tagging event (${event.event_id}) - ${event.message} - ${exc.type}: ${exc.value}`,
       });
     }
 
@@ -89,3 +87,11 @@ function addInternalBreadcrumb(arg: Parameters<typeof addBreadcrumb>[0]): void {
     ...rest,
   });
 }
+
+function getEventExceptionValues(event: Event): { type: string; value: string } {
+  return {
+    type: 'Unknown',
+    value: 'n/a',
+    ...(event.exception && event.exception.values && event.exception.values[0]),
+  };
+}

packages/replay/src/coreHandlers/handleXhr.ts

@@ -50,11 +50,15 @@ function handleXhr(handlerData: XhrHandlerData): ReplayPerformanceEntry | null {
     return null;
   }
 
+  const timestamp = handlerData.xhr.__sentry_xhr__
+    ? handlerData.xhr.__sentry_xhr__.startTimestamp || 0
+    : handlerData.endTimestamp;
+
   return {
     type: 'resource.xhr',
     name: url,
-    start: (handlerData.xhr.__sentry_xhr__?.startTimestamp || 0) / 1000 || handlerData.endTimestamp / 1000.0,
-    end: handlerData.endTimestamp / 1000.0,
+    start: timestamp / 1000,
+    end: handlerData.endTimestamp / 1000,
     data: {
       method,
       statusCode,

packages/replay/src/eventBuffer.ts

@@ -95,7 +95,7 @@ export class EventBufferCompressionWorker implements EventBuffer {
    */
   public _pendingEvents: RecordingEvent[] = [];
 
-  private _worker: null | Worker;
+  private _worker: Worker;
   private _eventBufferItemLength: number = 0;
   private _id: number = 0;
 
@@ -124,8 +124,7 @@ export class EventBufferCompressionWorker implements EventBuffer {
    */
   public destroy(): void {
     __DEBUG_BUILD__ && logger.log('[Replay] Destroying compression worker');
-    this._worker?.terminate();
-    this._worker = null;
+    this._worker.terminate();
   }
 
   /**
@@ -177,7 +176,7 @@ export class EventBufferCompressionWorker implements EventBuffer {
         }
 
         // At this point, we'll always want to remove listener regardless of result status
-        this._worker?.removeEventListener('message', listener);
+        this._worker.removeEventListener('message', listener);
 
         if (!data.success) {
           // TODO: Do some error handling, not sure what
@@ -200,8 +199,8 @@ export class EventBufferCompressionWorker implements EventBuffer {
 
       // Note: we can't use `once` option because it's possible it needs to
       // listen to multiple messages
-      this._worker?.addEventListener('message', listener);
-      this._worker?.postMessage({ id, method, args: stringifiedArgs });
+      this._worker.addEventListener('message', listener);
+      this._worker.postMessage({ id, method, args: stringifiedArgs });
     });
   }
 

packages/replay/src/replay.ts

@@ -231,8 +231,8 @@ export class ReplayContainer implements ReplayContainerInterface {
       __DEBUG_BUILD__ && logger.log('[Replay] Stopping Replays');
       this._isEnabled = false;
       this._removeListeners();
-      this._stopRecording?.();
-      this.eventBuffer?.destroy();
+      this._stopRecording && this._stopRecording();
+      this.eventBuffer && this.eventBuffer.destroy();
       this.eventBuffer = null;
       this._debouncedFlush.cancel();
     } catch (err) {
@@ -278,7 +278,7 @@ export class ReplayContainer implements ReplayContainerInterface {
    */
   public addUpdate(cb: AddUpdateCallback): void {
     // We need to always run `cb` (e.g. in the case of `this.recordingMode == 'error'`)
-    const cbResult = cb?.();
+    const cbResult = cb();
 
     // If this option is turned on then we will only want to call `flush`
     // explicitly
@@ -335,6 +335,11 @@ export class ReplayContainer implements ReplayContainerInterface {
     return this._debouncedFlush.flush() as Promise<void>;
   }
 
+  /** Get the current sesion (=replay) ID */
+  public getSessionId(): string | undefined {
+    return this.session && this.session.id;
+  }
+
   /** A wrapper to conditionally capture exceptions. */
   private _handleException(error: unknown): void {
     __DEBUG_BUILD__ && logger.error('[Replay]', error);
@@ -363,8 +368,9 @@ export class ReplayContainer implements ReplayContainerInterface {
       this._setInitialState();
     }
 
-    if (session.id !== this.session?.id) {
-      session.previousSessionId = this.session?.id;
+    const currentSessionId = this.getSessionId();
+    if (session.id !== currentSessionId) {
+      session.previousSessionId = currentSessionId;
     }
 
     this.session = session;
@@ -405,7 +411,9 @@ export class ReplayContainer implements ReplayContainerInterface {
       if (!this._hasInitializedCoreListeners) {
         // Listeners from core SDK //
         const scope = getCurrentHub().getScope();
-        scope?.addScopeListener(this._handleCoreBreadcrumbListener('scope'));
+        if (scope) {
+          scope.addScopeListener(this._handleCoreBreadcrumbListener('scope'));
+        }
         addInstrumentationHandler('dom', this._handleCoreBreadcrumbListener('dom'));
         addInstrumentationHandler('fetch', handleFetchSpanListener(this));
         addInstrumentationHandler('xhr', handleXhrSpanListener(this));
@@ -492,7 +500,7 @@ export class ReplayContainer implements ReplayContainerInterface {
       // of the previous session. Do not immediately flush in this case
       // to avoid capturing only the checkout and instead the replay will
       // be captured if they perform any follow-up actions.
-      if (this.session?.previousSessionId) {
+      if (this.session && this.session.previousSessionId) {
         return true;
       }
 
@@ -707,7 +715,7 @@ export class ReplayContainer implements ReplayContainerInterface {
    * Returns true if session is not expired, false otherwise.
    */
   private _checkAndHandleExpiredSession({ expiry = SESSION_IDLE_DURATION }: { expiry?: number } = {}): boolean | void {
-    const oldSessionId = this.session?.id;
+    const oldSessionId = this.getSessionId();
 
     // Prevent starting a new session if the last user activity is older than
     // MAX_SESSION_LIFE. Otherwise non-user activity can trigger a new
@@ -724,7 +732,7 @@ export class ReplayContainer implements ReplayContainerInterface {
     this._loadSession({ expiry });
 
     // Session was expired if session ids do not match
-    const expired = oldSessionId !== this.session?.id;
+    const expired = oldSessionId !== this.getSessionId();
 
     if (!expired) {
       return true;
@@ -788,20 +796,26 @@ export class ReplayContainer implements ReplayContainerInterface {
    * Should never be called directly, only by `flush`
    */
   private async _runFlush(): Promise<void> {
-    if (!this.session) {
-      __DEBUG_BUILD__ && logger.error('[Replay] No session found to flush.');
+    if (!this.session || !this.eventBuffer) {
+      __DEBUG_BUILD__ && logger.error('[Replay] No session or eventBuffer found to flush.');
       return;
     }
 
     await this._addPerformanceEntries();
 
-    if (!this.eventBuffer?.pendingLength) {
+    // Check eventBuffer again, as it could have been stopped in the meanwhile
+    if (!this.eventBuffer || !this.eventBuffer.pendingLength) {
       return;
     }
 
     // Only attach memory event if eventBuffer is not empty
     await addMemoryEntry(this);
 
+    // Check eventBuffer again, as it could have been stopped in the meanwhile
+    if (!this.eventBuffer) {
+      return;
+    }
+
     try {
       // Note this empties the event buffer regardless of outcome of sending replay
       const recordingData = await this.eventBuffer.finish();
@@ -853,13 +867,13 @@ export class ReplayContainer implements ReplayContainerInterface {
       return;
     }
 
-    if (!this.session?.id) {
+    if (!this.session) {
       __DEBUG_BUILD__ && logger.error('[Replay] No session found to flush.');
       return;
     }
 
     // A flush is about to happen, cancel any queued flushes
-    this._debouncedFlush?.cancel();
+    this._debouncedFlush.cancel();
 
     // this._flushLock acts as a lock so that future calls to `_flush()`
     // will be blocked until this promise resolves

packages/replay/src/types.ts

@@ -118,7 +118,7 @@ export interface ReplayPluginOptions extends SessionOptions {
    *
    * Default: undefined
    */
-  _experiments?: Partial<{
+  _experiments: Partial<{
     captureExceptions: boolean;
     traceInternals: boolean;
   }>;
@@ -259,6 +259,7 @@ export interface ReplayContainer {
   triggerUserActivity(): void;
   addUpdate(cb: AddUpdateCallback): void;
   getOptions(): ReplayPluginOptions;
+  getSessionId(): string | undefined;
 }
 
 export interface ReplayPerformanceEntry {

packages/replay/src/util/addEvent.ts

@@ -35,7 +35,7 @@ export async function addEvent(
   // Only record earliest event if a new session was created, otherwise it
   // shouldn't be relevant
   const earliestEvent = replay.getContext().earliestEvent;
-  if (replay.session?.segmentId === 0 && (!earliestEvent || timestampInMs < earliestEvent)) {
+  if (replay.session && replay.session.segmentId === 0 && (!earliestEvent || timestampInMs < earliestEvent)) {
     replay.getContext().earliestEvent = timestampInMs;
   }