Skip to content
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.

Sign up for GitHub

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: preserve search params in "onUnhandledRequest" messages #2128

Merged
merged 3 commits into from
Apr 17, 2024
Merged

fix: preserve search params in "onUnhandledRequest" messages #2128

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9dddafd
fix: preserving qparams in url if origin is same, which prints relati…
Apr 6, 2024
d50e764
fix: preserving search params by appending them to final publicUrl
nagadevkrishna Apr 12, 2024
04d355a
Merge branch 'main' into feature/preserve-original-req-url
kettanaito Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/core/utils/request/toPublicUrl.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ test('returns an absolute request URL withouth search params', () => {

expect(toPublicUrl(new URL('http://localhost/path'))).toBe('/path')

expect(toPublicUrl(new URL('http://localhost/path?foo=bar'))).toBe('/path')
expect(toPublicUrl(new URL('http://localhost/path?foo=bar'))).toBe(
'/path?foo=bar',
)
})

it('returns a relative URL given the request to the same origin', () => {
Expand Down
4 changes: 3 additions & 1 deletion src/core/utils/request/toPublicUrl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ export function toPublicUrl(url: string | URL): string {

const urlInstance = url instanceof URL ? url : new URL(url)

const [, relativeUrl] = urlInstance.href.split(urlInstance.origin)
Copy link
Member

@kettanaito kettanaito Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does solve the problem but it changes the other use cases we have for toPublicUrl.

I think a far better approach is to keep these changes scoped to the immediate problem area: the toPublic() usage in the onUnhandledRequest.ts.

msw/src/core/utils/request/onUnhandledRequest.ts

Lines 24 to 27 in e83a669

const url = new URL(request.url)
const publicUrl = toPublicUrl(url)
const unhandledRequestMessage = `intercepted a request without a matching request handler:\n\n \u2022 ${request.method} ${publicUrl}\n\nIf you still wish to intercept this unhandled request, please create a request handler for it.\nRead more: https://mswjs.io/docs/getting-started/mocks`

Let's try something like this:

const url = new URL(request.url)
const publicUrl = toPublicUrl(url) + url.search

This way both relative and absolute URLs will be printed correctly but will also have any search parameters preserved.

Then, we can modify the tests in onUnhandledRequest.test.ts to include two new scenarios:

  • Unhandled request with a relative URL and search params;
  • Unhandled request with an absolute URL and search params.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. Made changes now and pushed to PR again.


return urlInstance.origin === location.origin
? urlInstance.pathname
? relativeUrl
: urlInstance.origin + urlInstance.pathname
}