ref(utils): Use type predicates in is utility functions (#4124)

lobsterkatie · web-flow · commit b99ee25b1688 · 2021-11-05T11:40:01.000-07:00
We have a number of `is` utility functions (`isString`, `isError`, and so on), and while they tell the human reading the code something about the value in question, they don't tell TypeScript anything, because it doesn't see them as type checks. As a result, we end up with a lot of code like this: ``` if( isString(msg) ) { const words = (msg as string).split(' '); } ``` where we have to typecast our variables, despite their type already having been checked immediately beforehand. In order to fix this, TS lets you return a type predicate[1] from a type-checking function rather than a simple boolean, and that lets it know what the human reader knows - if `isString(msg)` is true, then `msg` must be a string. This switches all of our `is` functions to use type predicates, and removes all of the typecasts which are no longer necessary as a result of this change. [1] https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates
diff --git a/packages/browser/src/eventbuilder.ts b/packages/browser/src/eventbuilder.ts
@@ -65,15 +65,14 @@ export function eventFromUnknownInput(
 ): Event {
   let event: Event;
 
-  if (isErrorEvent(exception as ErrorEvent) && (exception as ErrorEvent).error) {
+  if (isErrorEvent(exception)) {
     // If it is an ErrorEvent with `error` property, extract it to get actual Error
-    const errorEvent = exception as ErrorEvent;
     // eslint-disable-next-line no-param-reassign
-    exception = errorEvent.error;
-    event = eventFromStacktrace(computeStackTrace(exception as Error));
+    exception = exception.error;
+    event = eventFromStacktrace(computeStackTrace(exception));
     return event;
   }
-  if (isDOMError(exception as DOMError) || isDOMException(exception as DOMException)) {
+  if (isDOMError(exception) || isDOMException(exception)) {
     // If it is a DOMError or DOMException (which are legacy APIs, but still supported in some browsers)
     // then we just extract the name, code, and message, as they don't provide anything else
     // https://developer.mozilla.org/en-US/docs/Web/API/DOMError
@@ -90,9 +89,9 @@ export function eventFromUnknownInput(
 
     return event;
   }
-  if (isError(exception as Error)) {
+  if (isError(exception)) {
     // we have a real Error object, do nothing
-    event = eventFromStacktrace(computeStackTrace(exception as Error));
+    event = eventFromStacktrace(computeStackTrace(exception));
     return event;
   }
   if (isPlainObject(exception) || isEvent(exception)) {
diff --git a/packages/integrations/src/ember.ts b/packages/integrations/src/ember.ts
@@ -58,7 +58,7 @@ export class Ember implements Integration {
         getCurrentHub().withScope(scope => {
           if (isInstanceOf(reason, Error)) {
             scope.setExtra('context', 'Unhandled Promise error detected');
-            getCurrentHub().captureException(reason, { originalException: reason as Error });
+            getCurrentHub().captureException(reason, { originalException: reason });
           } else {
             scope.setExtra('reason', reason);
             getCurrentHub().captureMessage('Unhandled Promise error detected');
diff --git a/packages/integrations/src/extraerrordata.ts b/packages/integrations/src/extraerrordata.ts
@@ -97,7 +97,7 @@ export class ExtraErrorData implements Integration {
           continue;
         }
         const value = error[key];
-        extraErrorInfo[key] = isError(value) ? (value as Error).toString() : value;
+        extraErrorInfo[key] = isError(value) ? value.toString() : value;
       }
 
       // Check if someone attached `toJSON` method to grab even more properties (eg. axios is doing that)
@@ -106,7 +106,7 @@ export class ExtraErrorData implements Integration {
 
         for (const key of Object.keys(serializedError)) {
           const value = serializedError[key];
-          extraErrorInfo[key] = isError(value) ? (value as Error).toString() : value;
+          extraErrorInfo[key] = isError(value) ? value.toString() : value;
         }
       }
 
diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts
@@ -227,7 +227,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler {
           // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision)
           let traceparentData;
           if (req.headers && isString(req.headers['sentry-trace'])) {
-            traceparentData = extractTraceparentData(req.headers['sentry-trace'] as string);
+            traceparentData = extractTraceparentData(req.headers['sentry-trace']);
             logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`);
           }
 
diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts
@@ -39,7 +39,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
           // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision)
           let traceparentData;
           if (req.headers && isString(req.headers['sentry-trace'])) {
-            traceparentData = extractTraceparentData(req.headers['sentry-trace'] as string);
+            traceparentData = extractTraceparentData(req.headers['sentry-trace']);
             logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`);
           }
 
diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts
@@ -41,7 +41,7 @@ export class NodeBackend extends BaseBackend<NodeOptions> {
         const message = `Non-Error exception captured with keys: ${extractExceptionKeysForMessage(exception)}`;
 
         getCurrentHub().configureScope(scope => {
-          scope.setExtra('__serialized__', normalizeToSize(exception as Record<string, unknown>));
+          scope.setExtra('__serialized__', normalizeToSize(exception));
         });
 
         ex = (hint && hint.syntheticException) || new Error(message);
diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts
@@ -56,7 +56,7 @@ export function tracingHandler(): (
     // If there is a trace header set, we extract the data from it (parentSpanId, traceId, and sampling decision)
     let traceparentData;
     if (req.headers && isString(req.headers['sentry-trace'])) {
-      traceparentData = extractTraceparentData(req.headers['sentry-trace'] as string);
+      traceparentData = extractTraceparentData(req.headers['sentry-trace']);
     }
 
     const transaction = startTransaction(
diff --git a/packages/serverless/src/awslambda.ts b/packages/serverless/src/awslambda.ts
@@ -256,7 +256,7 @@ export function wrapHandler<TEvent, TResult>(
     let traceparentData;
     const eventWithHeaders = event as { headers?: { [key: string]: string } };
     if (eventWithHeaders.headers && isString(eventWithHeaders.headers['sentry-trace'])) {
-      traceparentData = extractTraceparentData(eventWithHeaders.headers['sentry-trace'] as string);
+      traceparentData = extractTraceparentData(eventWithHeaders.headers['sentry-trace']);
     }
     const transaction = startTransaction({
       name: context.functionName,
diff --git a/packages/serverless/src/gcpfunction/http.ts b/packages/serverless/src/gcpfunction/http.ts
@@ -53,7 +53,7 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial<HttpFunctionWr
     let traceparentData;
     const reqWithHeaders = req as { headers?: { [key: string]: string } };
     if (reqWithHeaders.headers && isString(reqWithHeaders.headers['sentry-trace'])) {
-      traceparentData = extractTraceparentData(reqWithHeaders.headers['sentry-trace'] as string);
+      traceparentData = extractTraceparentData(reqWithHeaders.headers['sentry-trace']);
     }
     const transaction = startTransaction({
       name: `${reqMethod} ${reqUrl}`,
diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts
@@ -185,7 +185,7 @@ export function fetchCallback(
     const options = (handlerData.args[1] = (handlerData.args[1] as { [key: string]: any }) || {});
     let headers = options.headers;
     if (isInstanceOf(request, Request)) {
-      headers = (request as Request).headers;
+      headers = request.headers;
     }
     if (headers) {
       // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts
@@ -37,7 +37,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
     super(transactionContext);
 
     if (isInstanceOf(hub, Hub)) {
-      this._hub = hub as Hub;
+      this._hub = hub;
     }
 
     this.name = transactionContext.name || '';
diff --git a/packages/utils/src/is.ts b/packages/utils/src/is.ts
@@ -9,7 +9,7 @@ import { Primitive } from '@sentry/types';
  * @param wat A value to be checked.
  * @returns A boolean representing the result.
  */
-export function isError(wat: any): boolean {
+export function isError(wat: any): wat is Error {
   switch (Object.prototype.toString.call(wat)) {
     case '[object Error]':
       return true;
@@ -29,7 +29,7 @@ export function isError(wat: any): boolean {
  * @param wat A value to be checked.
  * @returns A boolean representing the result.
  */
-export function isErrorEvent(wat: any): boolean {
+export function isErrorEvent(wat: any): wat is ErrorEvent {
   return Object.prototype.toString.call(wat) === '[object ErrorEvent]';
 }
 
@@ -40,7 +40,7 @@ export function isErrorEvent(wat: any): boolean {
  * @param wat A value to be checked.
  * @returns A boolean representing the result.
  */
-export function isDOMError(wat: any): boolean {
+export function isDOMError(wat: any): wat is DOMError {
   return Object.prototype.toString.call(wat) === '[object DOMError]';
 }
 
@@ -51,18 +51,18 @@ export function isDOMError(wat: any): boolean {
  * @param wat A value to be checked.
  * @returns A boolean representing the result.
  */
-export function isDOMException(wat: any): boolean {
+export function isDOMException(wat: any): wat is DOMException {
   return Object.prototype.toString.call(wat) === '[object DOMException]';
 }
 
 /**
- * Checks whether given value's type is a string
+ * Checks whether the given value's type is string
  * {@link isString}.
  *
  * @param wat A value to be checked.
  * @returns A boolean representing the result.
  */
-export function isString(wat: any): boolean {
+export function isString(wat: any): wat is string {
   return Object.prototype.toString.call(wat) === '[object String]';
 }
 
@@ -84,7 +84,7 @@ export function isPrimitive(wat: any): wat is Primitive {
  * @param wat A value to be checked.
  * @returns A boolean representing the result.
  */
-export function isPlainObject(wat: any): boolean {
+export function isPlainObject(wat: any): wat is { [key: string]: unknown } {
   return Object.prototype.toString.call(wat) === '[object Object]';
 }
 
@@ -95,7 +95,7 @@ export function isPlainObject(wat: any): boolean {
  * @param wat A value to be checked.
  * @returns A boolean representing the result.
  */
-export function isEvent(wat: any): boolean {
+export function isEvent(wat: any): wat is Event {
   return typeof Event !== 'undefined' && isInstanceOf(wat, Event);
 }
 
@@ -106,7 +106,7 @@ export function isEvent(wat: any): boolean {
  * @param wat A value to be checked.
  * @returns A boolean representing the result.
  */
-export function isElement(wat: any): boolean {
+export function isElement(wat: any): wat is Element {
   return typeof Element !== 'undefined' && isInstanceOf(wat, Element);
 }
 
@@ -117,7 +117,7 @@ export function isElement(wat: any): boolean {
  * @param wat A value to be checked.
  * @returns A boolean representing the result.
  */
-export function isRegExp(wat: any): boolean {
+export function isRegExp(wat: any): wat is RegExp {
   return Object.prototype.toString.call(wat) === '[object RegExp]';
 }
 
@@ -140,6 +140,12 @@ export function isThenable(wat: any): boolean {
 export function isSyntheticEvent(wat: any): boolean {
   return isPlainObject(wat) && 'nativeEvent' in wat && 'preventDefault' in wat && 'stopPropagation' in wat;
 }
+
+// Having the two different types is necessary because some types packages type classes as functions (in addition to
+// being objects with a `new` method), and some only type them as `new`-able objects
+type Constructor<T> = (...args: any[]) => T;
+type ClassWithConstructor<T> = { new (...args: any[]): T };
+
 /**
  * Checks whether given value's type is an instance of provided constructor.
  * {@link isInstanceOf}.
@@ -148,7 +154,7 @@ export function isSyntheticEvent(wat: any): boolean {
  * @param base A constructor to be used in a check.
  * @returns A boolean representing the result.
  */
-export function isInstanceOf(wat: any, base: any): boolean {
+export function isInstanceOf<T>(wat: any, base: Constructor<T> | ClassWithConstructor<T>): wat is T {
   try {
     return wat instanceof base;
   } catch (_e) {
diff --git a/packages/utils/src/object.ts b/packages/utils/src/object.ts
@@ -103,7 +103,7 @@ function getWalkSource(
       currentTarget?: unknown;
     }
 
-    const event = value as SimpleEvent;
+    const event = (value as unknown) as SimpleEvent;
 
     const source: {
       [key: string]: any;
@@ -381,9 +381,8 @@ export function extractExceptionKeysForMessage(exception: any, maxLength: number
  * Given any object, return the new object with removed keys that value was `undefined`.
  * Works recursively on objects and arrays.
  */
-export function dropUndefinedKeys<T>(val: T): T {
-  if (isPlainObject(val)) {
-    const obj = val as { [key: string]: any };
+export function dropUndefinedKeys<T>(obj: T): T {
+  if (isPlainObject(obj)) {
     const rv: { [key: string]: any } = {};
     for (const key of Object.keys(obj)) {
       if (typeof obj[key] !== 'undefined') {
@@ -393,11 +392,11 @@ export function dropUndefinedKeys<T>(val: T): T {
     return rv as T;
   }
 
-  if (Array.isArray(val)) {
-    return (val as any[]).map(dropUndefinedKeys) as any;
+  if (Array.isArray(obj)) {
+    return (obj as any[]).map(dropUndefinedKeys) as any;
   }
 
-  return val;
+  return obj;
 }
 
 /**

Original file line number	Diff line number	Diff line change
`@@ -97,7 +97,7 @@ export class ExtraErrorData implements Integration {`
`97`	`97`	`continue;`
`98`	`98`	`}`
`99`	`99`	`const value = error[key];`
`100`		`- extraErrorInfo[key] = isError(value) ? (value as Error).toString() : value;`
	`100`	`+ extraErrorInfo[key] = isError(value) ? value.toString() : value;`
`101`	`101`	`}`
`102`	`102`
`103`	`103`	// Check if someone attached `toJSON` method to grab even more properties (eg. axios is doing that)
`@@ -106,7 +106,7 @@ export class ExtraErrorData implements Integration {`
`106`	`106`
`107`	`107`	`for (const key of Object.keys(serializedError)) {`
`108`	`108`	`const value = serializedError[key];`
`109`		`- extraErrorInfo[key] = isError(value) ? (value as Error).toString() : value;`
	`109`	`+ extraErrorInfo[key] = isError(value) ? value.toString() : value;`
`110`	`110`	`}`
`111`	`111`	`}`
`112`	`112`
Original file line number	Diff line number	Diff line change
`@@ -227,7 +227,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler {`
`227`	`227`	`// If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision)`
`228`	`228`	`let traceparentData;`
`229`	`229`	`if (req.headers && isString(req.headers['sentry-trace'])) {`
`230`		`- traceparentData = extractTraceparentData(req.headers['sentry-trace'] as string);`
	`230`	`+ traceparentData = extractTraceparentData(req.headers['sentry-trace']);`
`231`	`231`	logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`);
`232`	`232`	`}`
`233`	`233`
Original file line number	Diff line number	Diff line change
`@@ -39,7 +39,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =`
`39`	`39`	`// If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision)`
`40`	`40`	`let traceparentData;`
`41`	`41`	`if (req.headers && isString(req.headers['sentry-trace'])) {`
`42`		`- traceparentData = extractTraceparentData(req.headers['sentry-trace'] as string);`
	`42`	`+ traceparentData = extractTraceparentData(req.headers['sentry-trace']);`
`43`	`43`	logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`);
`44`	`44`	`}`
`45`	`45`
Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,7 @@ export function tracingHandler(): (`
`56`	`56`	`// If there is a trace header set, we extract the data from it (parentSpanId, traceId, and sampling decision)`
`57`	`57`	`let traceparentData;`
`58`	`58`	`if (req.headers && isString(req.headers['sentry-trace'])) {`
`59`		`- traceparentData = extractTraceparentData(req.headers['sentry-trace'] as string);`
	`59`	`+ traceparentData = extractTraceparentData(req.headers['sentry-trace']);`
`60`	`60`	`}`
`61`	`61`
`62`	`62`	`const transaction = startTransaction(`
Original file line number	Diff line number	Diff line change
`@@ -256,7 +256,7 @@ export function wrapHandler<TEvent, TResult>(`
`256`	`256`	`let traceparentData;`
`257`	`257`	`const eventWithHeaders = event as { headers?: { [key: string]: string } };`
`258`	`258`	`if (eventWithHeaders.headers && isString(eventWithHeaders.headers['sentry-trace'])) {`
`259`		`- traceparentData = extractTraceparentData(eventWithHeaders.headers['sentry-trace'] as string);`
	`259`	`+ traceparentData = extractTraceparentData(eventWithHeaders.headers['sentry-trace']);`
`260`	`260`	`}`
`261`	`261`	`const transaction = startTransaction({`
`262`	`262`	`name: context.functionName,`
Original file line number	Diff line number	Diff line change
`@@ -53,7 +53,7 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial<HttpFunctionWr`
`53`	`53`	`let traceparentData;`
`54`	`54`	`const reqWithHeaders = req as { headers?: { [key: string]: string } };`
`55`	`55`	`if (reqWithHeaders.headers && isString(reqWithHeaders.headers['sentry-trace'])) {`
`56`		`- traceparentData = extractTraceparentData(reqWithHeaders.headers['sentry-trace'] as string);`
	`56`	`+ traceparentData = extractTraceparentData(reqWithHeaders.headers['sentry-trace']);`
`57`	`57`	`}`
`58`	`58`	`const transaction = startTransaction({`
`59`	`59`	name: `${reqMethod} ${reqUrl}`,
Original file line number	Diff line number	Diff line change
`@@ -185,7 +185,7 @@ export function fetchCallback(`
`185`	`185`	`const options = (handlerData.args[1] = (handlerData.args[1] as { [key: string]: any }) \|\| {});`
`186`	`186`	`let headers = options.headers;`
`187`	`187`	`if (isInstanceOf(request, Request)) {`
`188`		`- headers = (request as Request).headers;`
	`188`	`+ headers = request.headers;`
`189`	`189`	`}`
`190`	`190`	`if (headers) {`
`191`	`191`	`// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access`