Skip to content

Commit fea5834

Browse files
onurtemizkanonurtemizkan
committed
Mark proxied handlers, add unit tests
1 parent d8216c1 commit fea5834

File tree

2 files changed

+311
-4
lines changed

2 files changed

+311
-4
lines changed

‎packages/react/src/reactrouterv6-compat-utils.tsx

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
} from '@sentry/browser';
1111
import type { Client, Integration, Span, TransactionSource } from '@sentry/core';
1212
import {
13+
addNonEnumerableProperty,
1314
debug,
1415
getActiveSpan,
1516
getClient,
@@ -105,13 +106,17 @@ function createAsyncHandlerProxy(
105106
route: RouteObject,
106107
handlerKey: string,
107108
): (...args: unknown[]) => unknown {
108-
return new Proxy(originalFunction, {
109+
const proxy = new Proxy(originalFunction, {
109110
apply(target: (...args: unknown[]) => unknown, thisArg, argArray) {
110111
const result = target.apply(thisArg, argArray);
111112
handleAsyncHandlerResult(result, route, handlerKey);
112113
return result;
113114
},
114115
});
116+
117+
addNonEnumerableProperty(proxy, '__sentry_proxied__', true);
118+
119+
return proxy;
115120
}
116121

117122
/**
@@ -122,7 +127,7 @@ export function checkRouteForAsyncHandler(route: RouteObject): void {
122127
if (route.handle && typeof route.handle === 'object') {
123128
for (const key of Object.keys(route.handle)) {
124129
const maybeFn = route.handle[key];
125-
if (typeof maybeFn === 'function') {
130+
if (typeof maybeFn === 'function' && !(maybeFn as { __sentry_proxied__?: boolean }).__sentry_proxied__) {
126131
route.handle[key] = createAsyncHandlerProxy(maybeFn, route, key);
127132
}
128133
}

‎packages/react/test/reactrouterv6-compat-utils.test.tsx

Lines changed: 304 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
import { describe, expect } from 'vitest';
2-
import { getNumberOfUrlSegments } from '../src/reactrouterv6-compat-utils';
1+
import { describe, expect, it, test } from 'vitest';
2+
import { checkRouteForAsyncHandler, getNumberOfUrlSegments } from '../src/reactrouterv6-compat-utils';
3+
import type { RouteObject } from '../src/types';
34

45
describe('getNumberOfUrlSegments', () => {
56
test.each([
@@ -11,3 +12,304 @@ describe('getNumberOfUrlSegments', () => {
1112
expect(getNumberOfUrlSegments(input)).toEqual(output);
1213
});
1314
});
15+
16+
describe('checkRouteForAsyncHandler', () => {
17+
it('should not create nested proxies when called multiple times on the same route', () => {
18+
const mockHandler = () => Promise.resolve([]);
19+
const route: RouteObject = {
20+
path: '/test',
21+
handle: {
22+
lazyChildren: mockHandler,
23+
},
24+
};
25+
26+
checkRouteForAsyncHandler(route);
27+
checkRouteForAsyncHandler(route);
28+
29+
const proxiedHandler = route.handle?.lazyChildren;
30+
expect(typeof proxiedHandler).toBe('function');
31+
expect(proxiedHandler).not.toBe(mockHandler);
32+
33+
expect((proxiedHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true);
34+
35+
const proxyHandler = (proxiedHandler as any)?.__sentry_proxied__;
36+
expect(proxyHandler).toBe(true);
37+
});
38+
39+
it('should handle routes without handle property', () => {
40+
const route: RouteObject = {
41+
path: '/test',
42+
};
43+
44+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
45+
});
46+
47+
it('should handle routes with non-function handle properties', () => {
48+
const route: RouteObject = {
49+
path: '/test',
50+
handle: {
51+
someData: 'not a function',
52+
},
53+
};
54+
55+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
56+
});
57+
58+
it('should handle routes with null/undefined handle properties', () => {
59+
const route: RouteObject = {
60+
path: '/test',
61+
handle: null as any,
62+
};
63+
64+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
65+
});
66+
67+
it('should handle routes with mixed function and non-function handle properties', () => {
68+
const mockHandler = () => Promise.resolve([]);
69+
const route: RouteObject = {
70+
path: '/test',
71+
handle: {
72+
lazyChildren: mockHandler,
73+
someData: 'not a function',
74+
anotherData: 123,
75+
},
76+
};
77+
78+
checkRouteForAsyncHandler(route);
79+
80+
const proxiedHandler = route.handle?.lazyChildren;
81+
expect(typeof proxiedHandler).toBe('function');
82+
expect(proxiedHandler).not.toBe(mockHandler);
83+
expect((proxiedHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true);
84+
85+
// Non-function properties should remain unchanged
86+
expect(route.handle?.someData).toBe('not a function');
87+
expect(route.handle?.anotherData).toBe(123);
88+
});
89+
90+
it('should handle nested routes with async handlers', () => {
91+
const parentHandler = () => Promise.resolve([]);
92+
const childHandler = () => Promise.resolve([]);
93+
94+
const route: RouteObject = {
95+
path: '/parent',
96+
handle: {
97+
lazyChildren: parentHandler,
98+
},
99+
children: [
100+
{
101+
path: '/child',
102+
handle: {
103+
lazyChildren: childHandler,
104+
},
105+
},
106+
],
107+
};
108+
109+
checkRouteForAsyncHandler(route);
110+
111+
// Check parent handler is proxied
112+
const proxiedParentHandler = route.handle?.lazyChildren;
113+
expect(typeof proxiedParentHandler).toBe('function');
114+
expect(proxiedParentHandler).not.toBe(parentHandler);
115+
expect((proxiedParentHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true);
116+
117+
// Check child handler is proxied
118+
const proxiedChildHandler = route.children?.[0]?.handle?.lazyChildren;
119+
expect(typeof proxiedChildHandler).toBe('function');
120+
expect(proxiedChildHandler).not.toBe(childHandler);
121+
expect((proxiedChildHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true);
122+
});
123+
124+
it('should handle deeply nested routes', () => {
125+
const level1Handler = () => Promise.resolve([]);
126+
const level2Handler = () => Promise.resolve([]);
127+
const level3Handler = () => Promise.resolve([]);
128+
129+
const route: RouteObject = {
130+
path: '/level1',
131+
handle: {
132+
lazyChildren: level1Handler,
133+
},
134+
children: [
135+
{
136+
path: '/level2',
137+
handle: {
138+
lazyChildren: level2Handler,
139+
},
140+
children: [
141+
{
142+
path: '/level3',
143+
handle: {
144+
lazyChildren: level3Handler,
145+
},
146+
},
147+
],
148+
},
149+
],
150+
};
151+
152+
checkRouteForAsyncHandler(route);
153+
154+
// Check all handlers are proxied
155+
expect((route.handle?.lazyChildren as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true);
156+
expect((route.children?.[0]?.handle?.lazyChildren as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(
157+
true,
158+
);
159+
expect(
160+
(route.children?.[0]?.children?.[0]?.handle?.lazyChildren as { __sentry_proxied__?: boolean }).__sentry_proxied__,
161+
).toBe(true);
162+
});
163+
164+
it('should handle routes with multiple async handlers', () => {
165+
const handler1 = () => Promise.resolve([]);
166+
const handler2 = () => Promise.resolve([]);
167+
const handler3 = () => Promise.resolve([]);
168+
169+
const route: RouteObject = {
170+
path: '/test',
171+
handle: {
172+
lazyChildren: handler1,
173+
asyncLoader: handler2,
174+
dataLoader: handler3,
175+
},
176+
};
177+
178+
checkRouteForAsyncHandler(route);
179+
180+
// Check all handlers are proxied
181+
expect((route.handle?.lazyChildren as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true);
182+
expect((route.handle?.asyncLoader as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true);
183+
expect((route.handle?.dataLoader as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true);
184+
});
185+
186+
it('should not re-proxy already proxied functions', () => {
187+
const mockHandler = () => Promise.resolve([]);
188+
const route: RouteObject = {
189+
path: '/test',
190+
handle: {
191+
lazyChildren: mockHandler,
192+
},
193+
};
194+
195+
// First call should proxy the function
196+
checkRouteForAsyncHandler(route);
197+
const firstProxiedHandler = route.handle?.lazyChildren;
198+
expect(firstProxiedHandler).not.toBe(mockHandler);
199+
expect((firstProxiedHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true);
200+
201+
// Second call should not create a new proxy
202+
checkRouteForAsyncHandler(route);
203+
const secondProxiedHandler = route.handle?.lazyChildren;
204+
expect(secondProxiedHandler).toBe(firstProxiedHandler); // Should be the same proxy
205+
expect((secondProxiedHandler as { __sentry_proxied__?: boolean }).__sentry_proxied__).toBe(true);
206+
});
207+
208+
it('should handle routes with empty children array', () => {
209+
const route: RouteObject = {
210+
path: '/test',
211+
children: [],
212+
};
213+
214+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
215+
});
216+
217+
it('should handle routes with undefined children', () => {
218+
const route: RouteObject = {
219+
path: '/test',
220+
children: undefined,
221+
};
222+
223+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
224+
});
225+
226+
it('should handle routes with null children', () => {
227+
const route: RouteObject = {
228+
path: '/test',
229+
children: null as any,
230+
};
231+
232+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
233+
});
234+
235+
it('should handle routes with non-array children', () => {
236+
const route: RouteObject = {
237+
path: '/test',
238+
children: 'not an array' as any,
239+
};
240+
241+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
242+
});
243+
244+
it('should handle routes with handle that is not an object', () => {
245+
const route: RouteObject = {
246+
path: '/test',
247+
handle: 'not an object' as any,
248+
};
249+
250+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
251+
});
252+
253+
it('should handle routes with handle that is null', () => {
254+
const route: RouteObject = {
255+
path: '/test',
256+
handle: null as any,
257+
};
258+
259+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
260+
});
261+
262+
it('should handle routes with handle that is undefined', () => {
263+
const route: RouteObject = {
264+
path: '/test',
265+
handle: undefined as any,
266+
};
267+
268+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
269+
});
270+
271+
it('should handle routes with handle that is a function', () => {
272+
const route: RouteObject = {
273+
path: '/test',
274+
handle: (() => {}) as any,
275+
};
276+
277+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
278+
});
279+
280+
it('should handle routes with handle that is a string', () => {
281+
const route: RouteObject = {
282+
path: '/test',
283+
handle: 'string handle' as any,
284+
};
285+
286+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
287+
});
288+
289+
it('should handle routes with handle that is a number', () => {
290+
const route: RouteObject = {
291+
path: '/test',
292+
handle: 42 as any,
293+
};
294+
295+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
296+
});
297+
298+
it('should handle routes with handle that is a boolean', () => {
299+
const route: RouteObject = {
300+
path: '/test',
301+
handle: true as any,
302+
};
303+
304+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
305+
});
306+
307+
it('should handle routes with handle that is an array', () => {
308+
const route: RouteObject = {
309+
path: '/test',
310+
handle: [] as any,
311+
};
312+
313+
expect(() => checkRouteForAsyncHandler(route)).not.toThrow();
314+
});
315+
});

0 commit comments

Comments
 (0)