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.

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

Add query params as per axios spec #19

Closed
wants to merge 1 commit into from

Conversation

Antman261
Copy link
Contributor

@Antman261 Antman261 commented Aug 6, 2021

This simply adds Axios query params into http-schemas.

This is a non-breaking extension to the http-schemas API. Confirmed working as the access logs produce this entry during testing:

Incoming request: /random-numbers
::ffff:127.0.0.1 - - [06/Aug/2021:14:33:32 +0000] "GET /api/random-numbers?foo=bar HTTP/1.1" 200 62 "-" "axios/0.21.1"

Addresses #18

withCredentials: options?.withCredentials ?? false,
});
// Create an axios client for making actual HTTP requests. Initialise it with the relevant given options, if any.
const axiosClient = axios.create(options);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no need to limit the options here, we can just use the Axios type and pass the options param thru, simplifies things a bit and allows users to use Axios features.

@yortus
Copy link
Owner

yortus commented Aug 8, 2021

Thanks for this @Antman261. I'm actually planning to drop the axios dep in next major version and just use the built-in fetch API under the hood, but keep the same public API. That's why I kept the HttpClientOptions decoupled from axios, so the public API doesn't couple us to their implementation. Do you think there is a compelling reason to stick with axios instead of using the standard fetch API under the hood?

The rest looks pretty good. I'd probably go with the slightly shorter query instead of queryParams for the new HttpRequest property. In future it could accept either a string or an object, although accepting just objects for now is probably best. The axios type of params seems to just be any atm, so I think the type for query also needs to be specified explicitly in our API so as not to be coupled to the axios impl.

@Antman261
Copy link
Contributor Author

In future it could accept either a string or an object or URLSearchParams? 🤔 which Axios accepts atm. Though it might be easier to just make it Record<string, string> | string for now, I think URLSearchParams adds complexity tbh. Even though its a good way of programmatically building query strings.

@yortus
Copy link
Owner

yortus commented Aug 17, 2021

URLSearchParams seems to be a very thin wrapper for a plain Map, not sure why they bothered. Probably the nuances of parsing the query string. But it's standard so fine to accept if you want to add it (or fine to leave out for now).

@Antman261 Antman261 closed this Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants