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

[proposal] array of querystring without number . #2025

Closed
alfa-jpn opened this issue Nov 25, 2017 · 10 comments
Closed

[proposal] array of querystring without number . #2025

alfa-jpn opened this issue Nov 25, 2017 · 10 comments
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code

Comments

@alfa-jpn
Copy link

Hi,
This is a breaking change proposal about a querystring parser and builder.

Current Behavior

The object is lost a type when use number with brackets.

query = m.buildQueryString({
  hash:  {0: "a", 1: "b"},
  array: ["a", "b"]
});
// > "hash%5B0%5D=a&hash%5B1%5D=b&array%5B0%5D=a&array%5B1%5D=b"
// > decode: "hash[0]=a&hash[1]=b&array[0]=a&array[1]=b"


m.parseQueryString(query)
// Both will convert to an array.
// > {hash:["a","b"],array:["a","b"]}

And, some servers do not handle query correctly.

for example,
The rack of ruby (ruby on rails) interpret a[0]=1&a[1]=2 as Hash.

Possible Solution

The querystring array does not use number with brackets.

query = m.buildQueryString({
  hash:  {0: "a", 1: "b"},
  array: ["a", "b"]
});
// > "hash%5B0%5D=a&hash%5B1%5D=b&array%5B%5D=a&array%5B%5D=b"
// > decode: "hash[0]=a&hash[1]=b&array[]=a&array[]=b"

m.parseQueryString(query)

// > {hash:{"1": "a", "2": "b"},array:["a","b"]}

Context

  • Prevent loss type.
  • Prevent problems between servers.

Your Environment

  • Version used: 1.1.5

Can i implement it and send PR?
Best regards,

@alfa-jpn alfa-jpn changed the title [proposal] array of query without number . [proposal] array of querystring without number . Nov 25, 2017
@dead-claudia
Copy link
Member

I'd back it up, considering I rarely see the foo[0]=whatever syntax outside of Mithril (I usually see foo[]=whatever).

@dead-claudia dead-claudia added the Type: Breaking Change For any feature request or suggestion that could reasonably break existing code label Nov 26, 2017
@alfa-jpn
Copy link
Author

alfa-jpn commented Dec 3, 2017

Thanks for a reply.

I tried implement it.
But i couldn't think of a good solution about parsing a nested array.
( https://github.com/MithrilJS/mithril.js/blob/next/querystring/tests/test-parseQueryString.js#L56 )

ex)
{a: [[1,2],[3,4]]} =[decode]=> "a[][]=1&a[][]=2&a[][]=3&a[][]=4" =[parse]=> ????

I think a nested array is unusual.
But should I keep it?

Best regards,

@dead-claudia
Copy link
Member

@alfa-jpn You should, since such functionality already exists. It should parse back to what you put into it: {a: [[1, 2], [3, 4]]}. FWIW, that's what Mithril does today with the indices.

@alfa-jpn
Copy link
Author

alfa-jpn commented Dec 3, 2017

@isaaclyman
Thanks for reply.
I understood the policy.

How about making it configurable?

ex)

m.queryString.arrayWithNumber= // config
m.queryString.parse() // moved from 'parseQueryString'
m.queryString.build() // moved from 'buildQueryString'


m.queryString.arrayWithNumber=true // existing behaviour (default)
m.queryString.build({a: [1,2,3]}) // return "a[0]=1&a[1]=2&a[2]=3"

m.queryString.arrayWithNumber=false // unsupported a nested array
m.queryString.build({a: [1,2,3]}) // return "a[]=1&a[]=2&a[]=3"

Best regards,

@isaaclyman
Copy link
Contributor

@alfa-jpn I believe you tagged me by mistake.

@pygy
Copy link
Member

pygy commented Dec 3, 2017

Sometimes, the github auto-completion has weird heuristics...

@dead-claudia
Copy link
Member

@alfa-jpn

How about making it configurable?

Eh, I'd rather not. Not for such a simple utility. Worst case scenario, the old version can find its way here, and migration should be relatively painless after making that substitution. It's pretty simple if you just read the source code.

@pygy

Sometimes, the github auto-completion has weird heuristics...

I'd have to vouch for that. At least it's not as bad as Gitter's, though (which I find myself frequently backspacing the name completed). 😄

@alfa-jpn
Copy link
Author

alfa-jpn commented Dec 10, 2017

@isaaclyman
Sorry for mistake. 🙇‍♂️


@isiahmeadows
Thanks for the advice.
I hope I can support old format with mithril-helper, Because I could not resolve the deep array issue.

I implemented it for reference.

Best regards,

@dead-claudia
Copy link
Member

@MithrilJS/collaborators This okay to shortlist for v2? We're kind of the exception here in generating and requiring integers. It's also a pretty simple one-line fix for buildQueryString and (I think) parseQueryString.

  • For buildQueryString, you replace key + "[" + i + "]" with key + "[]" here.
  • For parseQueryString, I think you remove the second condition, the !isNaN(parseInt(nextLevel, 10)) check, on this line.

@dead-claudia
Copy link
Member

Closing due to age.

@dead-claudia dead-claudia closed this as not planned Won't fix, can't repro, duplicate, stale Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code
Projects
Status: Completed/Declined
Development

No branches or pull requests

4 participants