feat: multi fill (#2028)

* feat: Add support for multiple fill calls while still calling all functions

* fix: Breadcrumb integration

Fixes #1981

* fix: Make multi patch optional

* fix: Remove multifill, not needed

* fix: autoflush

* meta: Changelog

* meta: CR

Author: HazAT

CHANGELOG.md

@@ -3,6 +3,8 @@
 ## Unreleased
 
 - [hub] feat: Add `setContext` on the scope
+- [browser] fix: Breacrumb integration ui clicks
+- [node] feat: Add `flushTimeout` to `requestHandler` to auto flush requests
 
 ## 5.0.8
 

packages/browser/src/integrations/breadcrumbs.ts

@@ -1,5 +1,5 @@
 import { API, getCurrentHub } from '@sentry/core';
-import { Breadcrumb, BreadcrumbHint, Integration, Severity } from '@sentry/types';
+import { Breadcrumb, BreadcrumbHint, Integration, Severity, WrappedFunction } from '@sentry/types';
 import {
   fill,
   getEventDescription,
@@ -19,7 +19,6 @@ import { breadcrumbEventHandler, keypressEventHandler, wrap } from './helpers';
 
 const global = getGlobalObject<Window>();
 let lastHref: string | undefined;
-
 /**
  * @hidden
  */
@@ -77,8 +76,7 @@ export class Breadcrumbs implements Integration {
     if (!('console' in global)) {
       return;
     }
-    const levels = ['log', 'info', 'warn', 'error', 'debug', 'assert'];
-    levels.forEach(function(level: string): void {
+    ['debug', 'info', 'warn', 'error', 'log', 'assert'].forEach(function(level: string): void {
       if (!(level in global.console)) {
         return;
       }
@@ -123,10 +121,82 @@ export class Breadcrumbs implements Integration {
     if (!('document' in global)) {
       return;
     }
+
     // Capture breadcrumbs from any click that is unhandled / bubbled up all the way
     // to the document. Do this before we instrument addEventListener.
     global.document.addEventListener('click', breadcrumbEventHandler('click'), false);
     global.document.addEventListener('keypress', keypressEventHandler(), false);
+
+    // After hooking into document bubbled up click and keypresses events, we also hook into user handled click & keypresses.
+    ['EventTarget', 'Node'].forEach((target: string) => {
+      const proto = (global as any)[target] && (global as any)[target].prototype;
+
+      if (!proto || !proto.hasOwnProperty || !proto.hasOwnProperty('addEventListener')) {
+        return;
+      }
+
+      fill(proto, 'addEventListener', function(
+        original: () => void,
+      ): (
+        eventName: string,
+        fn: EventListenerOrEventListenerObject,
+        options?: boolean | AddEventListenerOptions,
+      ) => void {
+        return function(
+          this: any,
+          eventName: string,
+          fn: EventListenerOrEventListenerObject,
+          options?: boolean | AddEventListenerOptions,
+        ): (eventName: string, fn: EventListenerOrEventListenerObject, capture?: boolean, secure?: boolean) => void {
+          if ((fn as any).handleEvent) {
+            if (eventName === 'click') {
+              fill(fn, 'handleEvent', function(innerOriginal: () => void): (caughtEvent: Event) => void {
+                return function(this: any, event: Event): (event: Event) => void {
+                  breadcrumbEventHandler('click')(event);
+                  return innerOriginal.call(this, event);
+                };
+              });
+            }
+            if (eventName === 'keypress') {
+              fill(fn, 'handleEvent', keypressEventHandler());
+            }
+          } else {
+            if (eventName === 'click') {
+              breadcrumbEventHandler('click', true)(this);
+            }
+            if (eventName === 'keypress') {
+              keypressEventHandler()(this);
+            }
+          }
+
+          return original.call(this, eventName, fn, options);
+        };
+      });
+
+      fill(proto, 'removeEventListener', function(
+        original: () => void,
+      ): (
+        this: any,
+        eventName: string,
+        fn: EventListenerObject,
+        options?: boolean | EventListenerOptions,
+      ) => () => void {
+        return function(
+          this: any,
+          eventName: string,
+          fn: EventListenerObject,
+          options?: boolean | EventListenerOptions,
+        ): () => void {
+          let callback = (fn as any) as WrappedFunction;
+          try {
+            callback = callback && (callback.__sentry_wrapped__ || callback);
+          } catch (e) {
+            // ignore, accessing __sentry_wrapped__ will throw in some Selenium environments
+          }
+          return original.call(this, eventName, callback, options);
+        };
+      });
+    });
   }
 
   /** JSDoc */

packages/browser/src/integrations/helpers.ts

@@ -153,49 +153,66 @@ export function wrap(
   return sentryWrapped;
 }
 
+let debounceTimer: number = 0;
+
 /**
  * Wraps addEventListener to capture UI breadcrumbs
  * @param eventName the event name (e.g. "click")
  * @returns wrapped breadcrumb events handler
  * @hidden
  */
-export function breadcrumbEventHandler(eventName: string): (event: Event) => void {
+export function breadcrumbEventHandler(eventName: string, debounce: boolean = false): (event: Event) => void {
   return (event: Event) => {
     // reset keypress timeout; e.g. triggering a 'click' after
     // a 'keypress' will reset the keypress debounce so that a new
     // set of keypresses can be recorded
     keypressTimeout = undefined;
-
     // It's possible this handler might trigger multiple times for the same
     // event (e.g. event propagation through node ancestors). Ignore if we've
     // already captured the event.
-    if (lastCapturedEvent === event) {
+    if (!event || lastCapturedEvent === event) {
       return;
     }
 
     lastCapturedEvent = event;
 
-    // try/catch both:
-    // - accessing event.target (see getsentry/raven-js#838, #768)
-    // - `htmlTreeAsString` because it's complex, and just accessing the DOM incorrectly
-    //   can throw an exception in some circumstances.
-    let target;
-    try {
-      target = _htmlTreeAsString(event.target as Node);
-    } catch (e) {
-      target = '<unknown>';
+    const captureBreadcrumb = () => {
+      // try/catch both:
+      // - accessing event.target (see getsentry/raven-js#838, #768)
+      // - `htmlTreeAsString` because it's complex, and just accessing the DOM incorrectly
+      //   can throw an exception in some circumstances.
+      let target;
+      try {
+        target = event.target ? _htmlTreeAsString(event.target as Node) : _htmlTreeAsString((event as unknown) as Node);
+      } catch (e) {
+        target = '<unknown>';
+      }
+
+      if (target.length === 0) {
+        return;
+      }
+
+      getCurrentHub().addBreadcrumb(
+        {
+          category: `ui.${eventName}`, // e.g. ui.click, ui.input
+          message: target,
+        },
+        {
+          event,
+          name: eventName,
+        },
+      );
+    };
+
+    if (debounceTimer) {
+      clearTimeout(debounceTimer);
     }
 
-    getCurrentHub().addBreadcrumb(
-      {
-        category: `ui.${eventName}`, // e.g. ui.click, ui.input
-        message: target,
-      },
-      {
-        event,
-        name: eventName,
-      },
-    );
+    if (debounce) {
+      debounceTimer = setTimeout(captureBreadcrumb);
+    } else {
+      captureBreadcrumb();
+    }
   };
 }
 

packages/browser/src/integrations/trycatch.ts

@@ -1,7 +1,7 @@
 import { Integration, WrappedFunction } from '@sentry/types';
 import { fill, getGlobalObject } from '@sentry/utils';
 
-import { breadcrumbEventHandler, keypressEventHandler, wrap } from './helpers';
+import { wrap } from './helpers';
 
 /** Wrap timer functions and event targets to catch errors and provide better meta data */
 export class TryCatch implements Integration {
@@ -85,60 +85,20 @@ export class TryCatch implements Integration {
           // can sometimes get 'Permission denied to access property "handle Event'
         }
 
-        // More breadcrumb DOM capture ... done here and not in `_instrumentBreadcrumbs`
-        // so that we don't have more than one wrapper function
-        let before: any;
-        let clickHandler: any;
-        let keypressHandler: any;
-
-        if (target === 'EventTarget' || target === 'Node') {
-          // NOTE: generating multiple handlers per addEventListener invocation, should
-          //       revisit and verify we can just use one (almost certainly)
-          clickHandler = breadcrumbEventHandler('click');
-          keypressHandler = keypressEventHandler();
-          before = function(event: Event): any {
-            // need to intercept every DOM event in `before` argument, in case that
-            // same wrapped method is re-used for different events (e.g. mousemove THEN click)
-            // see #724
-            if (!event) {
-              return;
-            }
-
-            let eventType;
-            try {
-              eventType = event.type;
-            } catch (e) {
-              // just accessing event properties can throw an exception in some rare circumstances
-              // see: https://github.com/getsentry/raven-js/issues/838
-              return;
-            }
-            if (eventType === 'click') {
-              return clickHandler(event);
-            }
-            if (eventType === 'keypress') {
-              return keypressHandler(event);
-            }
-          };
-        }
-
         return original.call(
           this,
           eventName,
-          wrap(
-            (fn as any) as WrappedFunction,
-            {
-              mechanism: {
-                data: {
-                  function: 'addEventListener',
-                  handler: getFunctionName(fn),
-                  target,
-                },
-                handled: true,
-                type: 'instrument',
+          wrap((fn as any) as WrappedFunction, {
+            mechanism: {
+              data: {
+                function: 'addEventListener',
+                handler: getFunctionName(fn),
+                target,
               },
+              handled: true,
+              type: 'instrument',
             },
-            before,
-          ),
+          }),
           options,
         );
       };

packages/node/src/handlers.ts

@@ -1,14 +1,14 @@
 import { captureException, getCurrentHub } from '@sentry/core';
 import { Event } from '@sentry/types';
 import { forget, isString, logger, normalize } from '@sentry/utils';
-import { flush } from './sdk';
 import * as cookie from 'cookie';
 import * as domain from 'domain';
 import * as http from 'http';
 import * as os from 'os';
 import * as url from 'url';
 
 import { NodeClient } from './client';
+import { flush } from './sdk';
 
 const DEFAULT_SHUTDOWN_TIMEOUT = 2000;
 
@@ -230,12 +230,17 @@ export function requestHandler(options?: {
     next: (error?: any) => void,
   ): void {
     if (options && options.flushTimeout && options.flushTimeout > 0) {
-      const _end = res.end
-
-      res.end = async function end (chunk?: any, encodingOrCb?: string | Function, cb?: Function) {
-        await flush(options.flushTimeout)
-        return _end.call(this, chunk, encodingOrCb, cb)
-      }
+      // tslint:disable-next-line: no-unbound-method
+      const _end = res.end;
+      res.end = function(chunk?: any | (() => void), encoding?: string | (() => void), cb?: () => void): void {
+        flush(options.flushTimeout)
+          .then(() => {
+            _end.call(this, chunk, encoding, cb);
+          })
+          .catch(e => {
+            logger.error(e);
+          });
+      };
     }
     const local = domain.create();
     local.add(req);

packages/utils/src/object.ts

@@ -11,7 +11,6 @@ import { Memo } from './memo';
  * @param replacement A function that should be used to wrap a given method.
  * @returns void
  */
-
 export function fill(source: { [key: string]: any }, name: string, replacement: (...args: any[]) => any): void {
   if (!(name in source)) {
     return;

packages/utils/test/object.test.ts

@@ -33,6 +33,30 @@ describe('fill()', () => {
     expect.assertions(3);
   });
 
+  test('mulitple fills calls all functions', () => {
+    const source = {
+      foo: (): number => 42,
+    };
+    const name = 'foo';
+    const replacement = jest.fn().mockImplementationOnce(cb => {
+      expect(cb).toBe(source.foo);
+      return () => 1337;
+    });
+
+    const replacement2 = jest.fn().mockImplementationOnce(cb => {
+      expect(cb).toBe(source.foo);
+      return () => 1338;
+    });
+
+    fill(source, name, replacement);
+    fill(source, name, replacement2);
+
+    expect(source.foo()).toEqual(1338);
+    expect(replacement).toBeCalled();
+    expect(replacement2).toBeCalled();
+    expect.assertions(5);
+  });
+
   test('internal flags shouldnt be enumerable', () => {
     const source = {
       foo: (): number => 42,