Support array query params #234
Conversation
`URI.decode_www_form` doesn’t support arrays but `Rack::Utils.parse_query` does
|
Thanks! Taking a look tomorrow |
There was a problem hiding this comment.
Thanks a bunch @jbennett!
I think we should depend on the Rack::Utils.parse_nested_query + Rack::Utils.build_nested_query combo. Left some comments to that extent. Let me know what you think!
lib/hotwire_combobox/helper.rb
Outdated
| @@ -115,7 +115,7 @@ def hw_first_page? | |||
|
|
|||
| def hw_uri_with_params(url_or_path, **params) | |||
| URI.parse(url_or_path).tap do |url_or_path| | |||
| query = URI.decode_www_form(url_or_path.query || "").to_h.merge(params.compact_blank.stringify_keys) | |||
| query = Rack::Utils.parse_query(url_or_path.query || "").merge(params.compact_blank.stringify_keys) | |||
There was a problem hiding this comment.
I didn't know Rack exposed this util. Nice!
Glancing at Rack's source code, it seems we should use parse_nested_query instead. Any reason to prefer parse_query over that?
Also, I noticed Rack exposes build_nested_query as well. What do you think about using that instead of URI.encode_www_form on the next line? Probably a good idea to use the pair together as they'll likely evolve together as well inside Rack.
There was a problem hiding this comment.
Yup, makes a lot of sense to use that same setoff parse/build and switched to the nested version.
|
|
||
| html = hw_paginated_combobox_options [], next_page: 2 | ||
|
|
||
| assert_attrs html, tag_name: "turbo-frame", src: "/foo?ary=1&ary=2&page=2&format=turbo_stream" |
There was a problem hiding this comment.
As far as I know, Rails prefers the ary[]=1&ary[]=2... convention for array/nested params. I believe we get that for free with Rack's build_nested_query. Are you depending on the ary=1&ary=2 convention instead?
I think to get a passing test using that notation we'd need to unescape the url encoding in the result. Something like:
assert_attrs CGI.unescape(html), tag_name: "turbo-frame", src: "/foo?ary[]=1&ary[]=2&page=2&format=turbo_stream"
There was a problem hiding this comment.
switched the test to include []
|
Excellent! Thanks @jbennett. Going out in the next release. |
URI.decode_www_formdoesn’t support arrays butRack::Utils.parse_querydoes. I'm passing a list of IDs to exclude which currently is reduced to only the final value:This also addresses issue #211