Closed kako-jun closed 4 years ago
@kako-jun thank you for the thoughtful and detailed feedback!
I agree that this behavior isn't very useful. Currently urlcat passes the parameters directly to the URLSearchParams
constructor, regardless of their types. URLSearchParams
converts arrays to comma-separated lists. But this doesn't work well for more complex cases and sometimes completely different behavior is desired.
If I understand correctly, you need the following:
URLSearchParams
for escapingURLSearchParams
for escaping.In my experience, different servers handle arrays passed in query parameters in three different ways:
{ p: [1, 2] }
→ p=%5B1%2C2%5D
{ p: [1, 2] }
→ p=1&p=2
{ p: [1, 2] }
→ p=1,2
Anything else should be stringified, as you said.
I think urlcat could support all three cases, with one of them being the default, and the others being config options.
What do you think about the following API?
urlcat('http://example.com', { p: [1, 2] }) // default behavior - we need to determine which one
urlcat('http://example.com', { p: [1, 2] }, { arrays: 'stringify' }) // case 1 - call JSON.stringify
urlcat('http://example.com', { p: [1, 2] }, { arrays: 'repeat' }) // case 2 - repeated key-value pairs
urlcat('http://example.com', { p: [1, 2] }, { arrays: 'comma' }) // case 3 - comma-separated list
I will make a list of popular backend frameworks and see which of the above 3 methods they support. This way I can decide which one should be the default based on data.
What do you think?
I agree with all of your ideas.
The following attitude is wonderful.
I will make a list of popular backend frameworks and see which of the above 3 methods they support.
default behavior - we need to determine which one
I look forward to the option being added.
If the new key name is arrays
, is the name of the key specifying behavior for object objects
?
Popular library qs supports four options(indices
,brackets
, repeat
, comma
) to stringify array values and bracket
option is required to communicate with rails server: Hash#to_query in Rails
So it must be great to support brackets
option like below.
urlcat('http://example.com', { p: [1, 2] }, { arrays: 'brackets' }) // case 4 - repeated key-value with brackets
@kako-jun yes, i think arrays
and objects
would be good, short names, though the qs library @polysiya linked above uses arrayFormat
which is a bit more descriptive. I can't yet decide between them.
@polysiya thank you, that's a very good library!
I'm thinking about simply just switching to it and using it under the hood. My only concern is the size: qs is 160KB unpacked, urlcat is 14.7KB. I've also found another query string library called query-string which is much smaller (31.8 KB) and can do the same.
The names of the format options are also much better than what I came up with so I'll definitely use those.
I've been thinking about a simple way to do project-level configuration for urlcat which will become necessary once we start using the config options mentioned above.
For a single urlcat call, you could pass the config object as a parameter, as discussed above:
urlcat('http://example.com', { p: [1, 2] }, { arrayFormat: 'repeat' })
But if you want to reuse the same configuration across your project, this can hurt readability. Reducing clutter and helping you write readable code is my primary goal with urlcat. So I thought we could provide a factory function called configure
:
In a file called, for example, urlcat-config.ts
:
import { configure } from 'urlcat';
export const urlcat = configure({ arrayFormat: 'repeat' });
Then you could import the pre-configured urlcat function elsewhere in your project:
import { urlcat } from '../urlcat-config';
urlcat('http://example.com', { p: [1, 2] }) // this will use arrayFormat: 'repeat'
The configure
function would look like this:
function configure(config: UrlcatConfig) {
return (/* ... normal urlcat arguments ... */) =>
urlcat(/* ... normal urlcat aruguments ... */, config);
}
I've thought about this a bit more and I think we should allow overrinding project-level config settings in individual calls to the configured function. So we could do something like:
function configure(projectConfig: UrlcatConfig) {
return (/* ... normal urlcat arguments ... */, config?: UrlcatConfig) =>
urlcat(/* ... normal urlcat aruguments ... */, { ...projectConfig, ...config });
}
@balazsbotond, can I offer my help for this issue? I've been following the discussion and it seems clear what needs to be done.
@SimonJang that sounds great, thank you! I've been thinking a bit more about this issue since I last commented. It's getting late now - let me gather my thoughts in the morning and write down what needs to be done in my opinion so we can discuss it.
So I think we need to support:
We need to determine the defaults for both arrays and objects - for this we need data about popular backend frameworks (js and non-js).
So I think that our options are the following:
1. use query-string under the hood
2. use qs under the hood
3. use our own implementation
Whichever of the above we choose, we need to define our own config object format, not just simply pass the config through to the internally used library. This would decouple urlcat from the library used and allow us to use something else later without changing the API.
We also need to provide a factory function for configuring urlcat.
@SimonJang Which option do you think we should choose? What do you think about the overall design?
@balazsbotond I'm in for the help in development.
@EngrKhizarIqbal thank you!
Could you please tell us what you think about the options in my previous comment?
If you have the time, your help would also be appreciated in finding examples of how backend frameworks expect array and object query params to be formatted.
As for the implementation, I think this is a single-person task and @SimonJang was here first, so he should be the one to do it.
You are welcome. I can help you with NodeJS and ASP.NET backend examples. and I'm not in for this issue only, happy to help you with any task for this repo.
Also, this will be my first contribution to any open source project since I started my dev career, so pardon me in advance, if I made mistakes in this journey.
@EngrKhizarIqbal thank you in advance for the examples! I will add some more issues soon, watch out for them!
Don't worry, I myself am also pretty new to open source :)
@balazsbotond I tend to use an established library instead of reinventing the wheel because, like you mentioned, it's battle-tested and proved itself in production environments. Unless we can do it better and faster, which is a non-trivial undertaking, I would suggest using an existing solution to solve this sub problem.
As for which one, I think that choice boils down to your vision of urlcat
. Does it need to be a 'batteries included' full blown lib or more like a fast and light utility lib? That's not a question I can answer :sweat_smile:
@SimonJang I agree, let's use an established library. Above all else, I want urlcat to be convenient, so I think qs is the better choice. I'm assigning this ticket to you now - thanks for your help!
@all-contributors please add @kako-jun for ideas
@all-contributors please add @polysiya for ideas
@balazsbotond
I've put up a pull request to add @polysiya! :tada:
@balazsbotond
I've updated the pull request to add @kako-jun! :tada:
@all-contributors please add @SimonJang for ideas
@balazsbotond
I've put up a pull request to add @SimonJang! :tada:
@SimonJang I agree, let's use an established library. Above all else, I want urlcat to be convenient, so I think qs is the better choice. I'm assigning this ticket to you now - thanks for your help!
I'll give it a go this weekend and submit a PR :rocket:
@all-contributors please add @SimonJang for code
@balazsbotond
I've put up a pull request to add @SimonJang! :tada:
I would like to thank everybody who helped design this feature (@kako-jun, @polysiya, @SimonJang) and @SimonJang for the excellent implementation. Closing this thread now - this will be shipped in the next major version in at most a couple of days.
The type
any
is expected not to cause an error, no matter what type is passed. In fact, I passed in various types, but urlcat didn't make an error, so I thought it was wonderful.However, the following behavior was observed. I don't know if these are intended.
array type
in:
out:
[
and]
are removed as a result of the array being stringized. It will then be encoded.object type
in:
out:
The object has been stringized to
[object Object]
. The string will then be encoded.Personally, this behavior still feels rough. :sweat_smile:
I think
The behavior above seems to be inconsistent with passing
number
andstring
. urlcat should encodearray
andobject
asstring
containing[
and{
.I think no need to parse
params
in detail. The following multi-level arguments can be supplied by the specification, so there is no end to it.I think no need to quit
any
. Because urlcat is sometimes called from JavaScript as well as TypeScript.