You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "Fine0830 (via GitHub)" <gi...@apache.org> on 2025/07/31 09:06:53 UTC
[PR] refactor: optimize the XHR interceptor to preserve the prototype chain by setting the prototype of custom constructor [skywalking-client-js]
Fine0830 opened a new pull request, #164:
URL: https://github.com/apache/skywalking-client-js/pull/164
Fixes https://github.com/apache/skywalking/issues/13395
Signed-off-by: Qiuxia Fan <qi...@apache.org>
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: optimize the XHR interceptor to preserve the prototype chain by setting the prototype of custom constructor [skywalking-client-js]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #164:
URL: https://github.com/apache/skywalking-client-js/pull/164
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: optimize the XHR interceptor to preserve the prototype chain by setting the prototype of custom constructor [skywalking-client-js]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: optimize the XHR interceptor to preserve the prototype chain by setting the prototype of custom constructor [skywalking-client-js]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #164:
URL: https://github.com/apache/skywalking-client-js/pull/164#issuecomment-3139172258
@Fine0830 please check the copilot comments?
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org