-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: XHR req method capture #1527
Changes from 6 commits
0ce7440
c983e81
8864c63
34a50a7
5b262e0
2d1ceaa
960e125
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -309,7 +309,7 @@ function initXhrObserver(cb: networkCallback, win: IWindow, options: Required<Ne | |
.then((entry) => { | ||
const requests = prepareRequest({ | ||
entry, | ||
method: req.method, | ||
method: method, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the fix... the method is provided to the XHR function when it is called so we should use it |
||
status: xhr?.status, | ||
networkRequest, | ||
start, | ||
|
@@ -386,7 +386,7 @@ function prepareRequest({ | |
timeOrigin, | ||
timestamp, | ||
method: method, | ||
initiatorType: entry ? (entry.initiatorType as InitiatorType) : initiatorType, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. flips the choice here... if we're providing an initiator we should use it in preference to the detected one they should always match |
||
initiatorType: initiatorType ? initiatorType : entry ? (entry.initiatorType as InitiatorType) : undefined, | ||
status, | ||
requestHeaders: networkRequest.requestHeaders, | ||
requestBody: networkRequest.requestBody, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these tests have a lot of duplication now, but I want to get this fix out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is pretty convincing, how would it have failed before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method = POST assertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep... it was always GET before (or at least overwhelmingly GET I didn't check which)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and for clarity I validated the test failed before the change 😊