Copilot commented on code in PR #164:
URL:
https://github.com/apache/skywalking-client-js/pull/164#discussion_r2244807032
##########
src/trace/interceptors/xhr.ts:
##########
@@ -168,10 +202,10 @@ export default function xhrInterceptor(options:
CustomOptionsType, segments: Seg
isError: event.detail.status === 0 || event.detail.status >= 400,
// when requests failed, the status is 0
parentSpanId: segment.spans.length - 1,
componentId: ComponentId,
- peer: responseURL.host,
+ peer: responseURL?.host || url.host,
tags: customConfig.detailMode
? customConfig.customTags
- ? [...tags, ...customConfig.customTags]
+ ? [...tags, ...(customConfig.customTags || [])]
Review Comment:
The spread operator creates a new array on every request, which could impact
performance in high-traffic applications. Consider pre-computing the combined
tags array when customConfig changes, or use array concatenation methods that
are more efficient.
```suggestion
? customConfig.combinedTags
```
##########
src/trace/interceptors/xhr.ts:
##########
@@ -81,28 +103,36 @@ export default function xhrInterceptor(options:
CustomOptionsType, segments: Seg
traceSegmentId: '',
} as SegmentFields;
const xhrState = event.detail.readyState;
- const config = event.detail.getRequestConfig;
- let url = {} as URL;
- if (config[1].startsWith('http://') || config[1].startsWith('https://')) {
- url = new URL(config[1]);
- } else if (config[1].startsWith('//')) {
- url = new URL(`${window.location.protocol}${config[1]}`);
- } else {
- url = new URL(window.location.href);
- url.pathname = config[1];
+ const config = (event.detail.getRequestConfig || []) as IArguments;
Review Comment:
The type assertion `as IArguments` is unsafe here because the fallback `[]`
is not actually of type IArguments. This could lead to runtime errors when
accessing array indices. Consider using a more specific type or proper type
guards.
```suggestion
const config = isIArguments(event.detail.getRequestConfig) ?
event.detail.getRequestConfig : { length: 0, callee: undefined,
[Symbol.iterator]: Array.prototype[Symbol.iterator] };
```
##########
src/trace/interceptors/xhr.ts:
##########
@@ -50,29 +57,44 @@ export default function xhrInterceptor(options:
CustomOptionsType, segments: Seg
false,
);
+ // Override the open method to capture request configuration
liveXHR.open = function (
method: string,
url: string,
async: boolean,
username?: string | null,
password?: string | null,
) {
- this.getRequestConfig = arguments;
-
- return xhrOpen.apply(this, arguments);
+ // Store the request configuration for later use in tracing
+ (this as EnhancedXMLHttpRequest).getRequestConfig = arguments;
+
+ // Call the original open method with the correct context
+ return originalOpen.apply(this, arguments);
};
+
+ // Override the send method and keeping original functionality
liveXHR.send = function (body?: Document | BodyInit | null) {
- return xhrSend.apply(this, arguments);
+ return originalSend.apply(this, arguments);
};
return liveXHR;
}
+ // Preserve the prototype chain by setting the prototype of our custom
constructor
+ customizedXHR.prototype = originalXHR.prototype;
+
+ // Ensure the constructor property points to our custom constructor
+ customizedXHR.prototype.constructor = customizedXHR;
Review Comment:
Setting the constructor property after modifying the prototype chain could
be overwritten by subsequent prototype operations. Consider setting this
property as non-writable using Object.defineProperty() to ensure it remains
stable.
```suggestion
// Ensure the constructor property points to our custom constructor and
make it non-writable
Object.defineProperty(customizedXHR.prototype, 'constructor', {
value: customizedXHR,
writable: false,
configurable: true,
enumerable: false,
});
```
##########
src/trace/interceptors/xhr.ts:
##########
@@ -20,28 +20,35 @@ import { encode } from 'js-base64';
import { CustomOptionsType } from '../../types';
import { SegmentFields, SpanFields } from '../type';
-let customConfig: CustomOptionsType | any = {};
+interface EnhancedXMLHttpRequest extends XMLHttpRequest {
+ getRequestConfig?: IArguments;
Review Comment:
Using IArguments type for getRequestConfig is problematic because IArguments
is deprecated and doesn't provide type safety. Consider using a more specific
type like `[string, string, boolean?, string?, string?]` to match the
XMLHttpRequest.open() parameters.
```suggestion
getRequestConfig?: [string, string, boolean?, string?, string?];
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
