Skip to content

Commit bd3f72e

Browse files
HazATHazAT
authored
feat: multi fill (getsentry#2028)
* feat: Add support for multiple fill calls while still calling all functions * fix: Breadcrumb integration Fixes getsentry#1981 * fix: Make multi patch optional * fix: Remove multifill, not needed * fix: autoflush * meta: Changelog * meta: CR
1 parent e1b9900 commit bd3f72e

File tree

7 files changed

+161
-84
lines changed

7 files changed

+161
-84
lines changed

‎CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
## Unreleased
44

55
- [hub] feat: Add `setContext` on the scope
6+
- [browser] fix: Breacrumb integration ui clicks
7+
- [node] feat: Add `flushTimeout` to `requestHandler` to auto flush requests
68

79
## 5.0.8
810

‎packages/browser/src/integrations/breadcrumbs.ts

Lines changed: 74 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { API, getCurrentHub } from '@sentry/core';
2-
import { Breadcrumb, BreadcrumbHint, Integration, Severity } from '@sentry/types';
2+
import { Breadcrumb, BreadcrumbHint, Integration, Severity, WrappedFunction } from '@sentry/types';
33
import {
44
fill,
55
getEventDescription,
@@ -19,7 +19,6 @@ import { breadcrumbEventHandler, keypressEventHandler, wrap } from './helpers';
1919

2020
const global = getGlobalObject<Window>();
2121
let lastHref: string | undefined;
22-
2322
/**
2423
* @hidden
2524
*/
@@ -77,8 +76,7 @@ export class Breadcrumbs implements Integration {
7776
if (!('console' in global)) {
7877
return;
7978
}
80-
const levels = ['log', 'info', 'warn', 'error', 'debug', 'assert'];
81-
levels.forEach(function(level: string): void {
79+
['debug', 'info', 'warn', 'error', 'log', 'assert'].forEach(function(level: string): void {
8280
if (!(level in global.console)) {
8381
return;
8482
}
@@ -123,10 +121,82 @@ export class Breadcrumbs implements Integration {
123121
if (!('document' in global)) {
124122
return;
125123
}
124+
126125
// Capture breadcrumbs from any click that is unhandled / bubbled up all the way
127126
// to the document. Do this before we instrument addEventListener.
128127
global.document.addEventListener('click', breadcrumbEventHandler('click'), false);
129128
global.document.addEventListener('keypress', keypressEventHandler(), false);
129+
130+
// After hooking into document bubbled up click and keypresses events, we also hook into user handled click & keypresses.
131+
['EventTarget', 'Node'].forEach((target: string) => {
132+
const proto = (global as any)[target] && (global as any)[target].prototype;
133+
134+
if (!proto || !proto.hasOwnProperty || !proto.hasOwnProperty('addEventListener')) {
135+
return;
136+
}
137+
138+
fill(proto, 'addEventListener', function(
139+
original: () => void,
140+
): (
141+
eventName: string,
142+
fn: EventListenerOrEventListenerObject,
143+
options?: boolean | AddEventListenerOptions,
144+
) => void {
145+
return function(
146+
this: any,
147+
eventName: string,
148+
fn: EventListenerOrEventListenerObject,
149+
options?: boolean | AddEventListenerOptions,
150+
): (eventName: string, fn: EventListenerOrEventListenerObject, capture?: boolean, secure?: boolean) => void {
151+
if ((fn as any).handleEvent) {
152+
if (eventName === 'click') {
153+
fill(fn, 'handleEvent', function(innerOriginal: () => void): (caughtEvent: Event) => void {
154+
return function(this: any, event: Event): (event: Event) => void {
155+
breadcrumbEventHandler('click')(event);
156+
return innerOriginal.call(this, event);
157+
};
158+
});
159+
}
160+
if (eventName === 'keypress') {
161+
fill(fn, 'handleEvent', keypressEventHandler());
162+
}
163+
} else {
164+
if (eventName === 'click') {
165+
breadcrumbEventHandler('click', true)(this);
166+
}
167+
if (eventName === 'keypress') {
168+
keypressEventHandler()(this);
169+
}
170+
}
171+
172+
return original.call(this, eventName, fn, options);
173+
};
174+
});
175+
176+
fill(proto, 'removeEventListener', function(
177+
original: () => void,
178+
): (
179+
this: any,
180+
eventName: string,
181+
fn: EventListenerObject,
182+
options?: boolean | EventListenerOptions,
183+
) => () => void {
184+
return function(
185+
this: any,
186+
eventName: string,
187+
fn: EventListenerObject,
188+
options?: boolean | EventListenerOptions,
189+
): () => void {
190+
let callback = (fn as any) as WrappedFunction;
191+
try {
192+
callback = callback && (callback.__sentry_wrapped__ || callback);
193+
} catch (e) {
194+
// ignore, accessing __sentry_wrapped__ will throw in some Selenium environments
195+
}
196+
return original.call(this, eventName, callback, options);
197+
};
198+
});
199+
});
130200
}
131201

132202
/** JSDoc */

‎packages/browser/src/integrations/helpers.ts

Lines changed: 39 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -153,49 +153,66 @@ export function wrap(
153153
return sentryWrapped;
154154
}
155155

156+
let debounceTimer: number = 0;
157+
156158
/**
157159
* Wraps addEventListener to capture UI breadcrumbs
158160
* @param eventName the event name (e.g. "click")
159161
* @returns wrapped breadcrumb events handler
160162
* @hidden
161163
*/
162-
export function breadcrumbEventHandler(eventName: string): (event: Event) => void {
164+
export function breadcrumbEventHandler(eventName: string, debounce: boolean = false): (event: Event) => void {
163165
return (event: Event) => {
164166
// reset keypress timeout; e.g. triggering a 'click' after
165167
// a 'keypress' will reset the keypress debounce so that a new
166168
// set of keypresses can be recorded
167169
keypressTimeout = undefined;
168-
169170
// It's possible this handler might trigger multiple times for the same
170171
// event (e.g. event propagation through node ancestors). Ignore if we've
171172
// already captured the event.
172-
if (lastCapturedEvent === event) {
173+
if (!event || lastCapturedEvent === event) {
173174
return;
174175
}
175176

176177
lastCapturedEvent = event;
177178

178-
// try/catch both:
179-
// - accessing event.target (see getsentry/raven-js#838, #768)
180-
// - `htmlTreeAsString` because it's complex, and just accessing the DOM incorrectly
181-
// can throw an exception in some circumstances.
182-
let target;
183-
try {
184-
target = _htmlTreeAsString(event.target as Node);
185-
} catch (e) {
186-
target = '<unknown>';
179+
const captureBreadcrumb = () => {
180+
// try/catch both:
181+
// - accessing event.target (see getsentry/raven-js#838, #768)
182+
// - `htmlTreeAsString` because it's complex, and just accessing the DOM incorrectly
183+
// can throw an exception in some circumstances.
184+
let target;
185+
try {
186+
target = event.target ? _htmlTreeAsString(event.target as Node) : _htmlTreeAsString((event as unknown) as Node);
187+
} catch (e) {
188+
target = '<unknown>';
189+
}
190+
191+
if (target.length === 0) {
192+
return;
193+
}
194+
195+
getCurrentHub().addBreadcrumb(
196+
{
197+
category: `ui.${eventName}`, // e.g. ui.click, ui.input
198+
message: target,
199+
},
200+
{
201+
event,
202+
name: eventName,
203+
},
204+
);
205+
};
206+
207+
if (debounceTimer) {
208+
clearTimeout(debounceTimer);
187209
}
188210

189-
getCurrentHub().addBreadcrumb(
190-
{
191-
category: `ui.${eventName}`, // e.g. ui.click, ui.input
192-
message: target,
193-
},
194-
{
195-
event,
196-
name: eventName,
197-
},
198-
);
211+
if (debounce) {
212+
debounceTimer = setTimeout(captureBreadcrumb);
213+
} else {
214+
captureBreadcrumb();
215+
}
199216
};
200217
}
201218

‎packages/browser/src/integrations/trycatch.ts

Lines changed: 10 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { Integration, WrappedFunction } from '@sentry/types';
22
import { fill, getGlobalObject } from '@sentry/utils';
33

4-
import { breadcrumbEventHandler, keypressEventHandler, wrap } from './helpers';
4+
import { wrap } from './helpers';
55

66
/** Wrap timer functions and event targets to catch errors and provide better meta data */
77
export class TryCatch implements Integration {
@@ -85,60 +85,20 @@ export class TryCatch implements Integration {
8585
// can sometimes get 'Permission denied to access property "handle Event'
8686
}
8787

88-
// More breadcrumb DOM capture ... done here and not in `_instrumentBreadcrumbs`
89-
// so that we don't have more than one wrapper function
90-
let before: any;
91-
let clickHandler: any;
92-
let keypressHandler: any;
93-
94-
if (target === 'EventTarget' || target === 'Node') {
95-
// NOTE: generating multiple handlers per addEventListener invocation, should
96-
// revisit and verify we can just use one (almost certainly)
97-
clickHandler = breadcrumbEventHandler('click');
98-
keypressHandler = keypressEventHandler();
99-
before = function(event: Event): any {
100-
// need to intercept every DOM event in `before` argument, in case that
101-
// same wrapped method is re-used for different events (e.g. mousemove THEN click)
102-
// see #724
103-
if (!event) {
104-
return;
105-
}
106-
107-
let eventType;
108-
try {
109-
eventType = event.type;
110-
} catch (e) {
111-
// just accessing event properties can throw an exception in some rare circumstances
112-
// see: https://github.com/getsentry/raven-js/issues/838
113-
return;
114-
}
115-
if (eventType === 'click') {
116-
return clickHandler(event);
117-
}
118-
if (eventType === 'keypress') {
119-
return keypressHandler(event);
120-
}
121-
};
122-
}
123-
12488
return original.call(
12589
this,
12690
eventName,
127-
wrap(
128-
(fn as any) as WrappedFunction,
129-
{
130-
mechanism: {
131-
data: {
132-
function: 'addEventListener',
133-
handler: getFunctionName(fn),
134-
target,
135-
},
136-
handled: true,
137-
type: 'instrument',
91+
wrap((fn as any) as WrappedFunction, {
92+
mechanism: {
93+
data: {
94+
function: 'addEventListener',
95+
handler: getFunctionName(fn),
96+
target,
13897
},
98+
handled: true,
99+
type: 'instrument',
139100
},
140-
before,
141-
),
101+
}),
142102
options,
143103
);
144104
};

‎packages/node/src/handlers.ts

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
import { captureException, getCurrentHub } from '@sentry/core';
22
import { Event } from '@sentry/types';
33
import { forget, isString, logger, normalize } from '@sentry/utils';
4-
import { flush } from './sdk';
54
import * as cookie from 'cookie';
65
import * as domain from 'domain';
76
import * as http from 'http';
87
import * as os from 'os';
98
import * as url from 'url';
109

1110
import { NodeClient } from './client';
11+
import { flush } from './sdk';
1212

1313
const DEFAULT_SHUTDOWN_TIMEOUT = 2000;
1414

@@ -230,12 +230,17 @@ export function requestHandler(options?: {
230230
next: (error?: any) => void,
231231
): void {
232232
if (options && options.flushTimeout && options.flushTimeout > 0) {
233-
const _end = res.end
234-
235-
res.end = async function end (chunk?: any, encodingOrCb?: string | Function, cb?: Function) {
236-
await flush(options.flushTimeout)
237-
return _end.call(this, chunk, encodingOrCb, cb)
238-
}
233+
// tslint:disable-next-line: no-unbound-method
234+
const _end = res.end;
235+
res.end = function(chunk?: any | (() => void), encoding?: string | (() => void), cb?: () => void): void {
236+
flush(options.flushTimeout)
237+
.then(() => {
238+
_end.call(this, chunk, encoding, cb);
239+
})
240+
.catch(e => {
241+
logger.error(e);
242+
});
243+
};
239244
}
240245
const local = domain.create();
241246
local.add(req);

‎packages/utils/src/object.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import { Memo } from './memo';
1111
* @param replacement A function that should be used to wrap a given method.
1212
* @returns void
1313
*/
14-
1514
export function fill(source: { [key: string]: any }, name: string, replacement: (...args: any[]) => any): void {
1615
if (!(name in source)) {
1716
return;

‎packages/utils/test/object.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,30 @@ describe('fill()', () => {
3333
expect.assertions(3);
3434
});
3535

36+
test('mulitple fills calls all functions', () => {
37+
const source = {
38+
foo: (): number => 42,
39+
};
40+
const name = 'foo';
41+
const replacement = jest.fn().mockImplementationOnce(cb => {
42+
expect(cb).toBe(source.foo);
43+
return () => 1337;
44+
});
45+
46+
const replacement2 = jest.fn().mockImplementationOnce(cb => {
47+
expect(cb).toBe(source.foo);
48+
return () => 1338;
49+
});
50+
51+
fill(source, name, replacement);
52+
fill(source, name, replacement2);
53+
54+
expect(source.foo()).toEqual(1338);
55+
expect(replacement).toBeCalled();
56+
expect(replacement2).toBeCalled();
57+
expect.assertions(5);
58+
});
59+
3660
test('internal flags shouldnt be enumerable', () => {
3761
const source = {
3862
foo: (): number => 42,

0 commit comments

Comments
 (0)