[main] use proper URL for tracking when fetch is passed an empty string #2165
Conversation
extensions/applicationinsights-dependencies-js/Tests/Unit/src/ajax.tests.ts
Outdated
Show resolved
Hide resolved
jorupp
force-pushed
the
bugfix/2164-fetch-empty-string-tracking
branch
from
cb703c2
to
2ce78e7
Compare
…empty string This allows AI to track it and sets the Traceparent header
jorupp
force-pushed
the
bugfix/2164-fetch-empty-string-tracking
branch
from
2ce78e7
to
6ee8dd4
Compare
| @@ -1186,6 +1186,9 @@ export class AjaxMonitor extends BaseTelemetryPlugin implements IDependenciesPlu | |||
|
|
|||
| if (input instanceof Request) { | |||
| ajaxData.requestUrl = input ? input.url : ""; | |||
| } else if (input === "" ) { | |||
| const location = getLocation(); | |||
There was a problem hiding this comment.
this seemed to be the way existing code was accessing window.location
There was a problem hiding this comment.
Ok, now this makes a little more sense, your using an empty URL "" So that you can talk back to the same domain as the hosting page regardless of which environment you are deployed in (or page your hosted on)
There was a problem hiding this comment.
well, if i wrote the code, I wouldn't have used "", but that's how NextJS 13's app router handles server-actions - I brought it up in a discussions there, but after digging further into the code that appeared to be causing the issue (https://github.com/vercel/next.js/blob/82eb6e6f4955dc90752a4875ccf86b7ac0c8d03a/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts#L58), it didn't seem that what the Next code was doing was actually contrary to how fetch is documented (or actually) works, so I figured the fix was better on this side in case there were other apps/libraries taking advantage of that behavior for "" with fetch.
| @@ -1186,6 +1186,9 @@ export class AjaxMonitor extends BaseTelemetryPlugin implements IDependenciesPlu | |||
|
|
|||
| if (input instanceof Request) { | |||
| ajaxData.requestUrl = input ? input.url : ""; | |||
| } else if (input === "" ) { | |||
| const location = getLocation(); | |||
| ajaxData.requestUrl = location?.href?.split("#")[0] || input; | |||
There was a problem hiding this comment.
We tend to not use the TypeScript ?. operator as this causes a lot of JavaScript to get generated and we are always attempting to keep the size of the generated JS code to a minimum.
Suggest maybe something like the following so that we handle the case where input.url === ""
let requestUrl: string;
if (input instanceof Request) {
requestUrl = (input||{}).url || "";
} else {
requestUrl = input;
}
// Handle case where either the input (as a string) or the input.url is an empty string
if (requestUrl === "") {
const location = getLocation();
if (location && location.href) {
requestUrl = strSplit(location.href, "#")[0];
}
}
ajaxData.requestUrl = requestUrl;Where strSplit is imported from @nevware21/ts-utils
There was a problem hiding this comment.
ok, will update. I saw a couple other uses of ?. in this file so I thought it ok here.
There was a problem hiding this comment.
ah, so not a "does it work" thing, but "how much JS gets generated" - got it.
There was a problem hiding this comment.
oh, that's a good point too re: "what if input.url === """ - I'll test how that actually acts.
There was a problem hiding this comment.
new Request("") appears to be setting url to the current URL itself, but it doesn't hurt to handle that here.
There was a problem hiding this comment.
ah, so not a "does it work" thing, but "how much JS gets generated" - got it.
Will need to create a work item to track those down and remove
|
Thanks again for creating the PR to address this issue |
|
Going to close and re-open to force the CLA to rerun |
This allows AI to track the dependency and sets the Traceparent header on the request to the server (if otherwise allowed).
Traceparentis now setFully-populated dependency is sent to ApplicationInsights
Data is displayed in Application Insights
Fixes #2164