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

Support array query params #234

Merged
merged 2 commits into from
Jan 1, 2025
Merged

Support array query params #234

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2d55df
Support array query params
jbennett Dec 21, 2024
40b8f47
use Rack::Util for parsing and building URLs
jbennett Jan 1, 2025
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
2 changes: 1 addition & 1 deletion lib/hotwire_combobox/helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, makes a lot of sense to use that same setoff parse/build and switched to the nested version.

url_or_path.query = URI.encode_www_form query
end.to_s
rescue URI::InvalidURIError
Expand Down
9 changes: 9 additions & 0 deletions test/helpers/hotwire_combobox/helper_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,15 @@ class HotwireCombobox::HelperTest < ApplicationViewTestCase
assert_attrs html, tag_name: "turbo-frame", src: "/foo?bar=baz&qux=quux&page=2&format=turbo_stream"
end

test "hw_paginated_combobox_options includes array params" do
request.expects(:path).returns("/foo")
request.expects(:query_parameters).returns({ ary: [ 1, 2 ] }.with_indifferent_access)

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"
Copy link
Owner

Choose a reason for hiding this comment

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

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switched the test to include []

end

test "single repeating character values" do
form = form_with model: OpenStruct.new(run_at: "* * * * *", persisted?: true, model_name: OpenStruct.new(param_key: :foo)), url: "#" do |form|
form.combobox :run_at, [ "@hourly", "@daily", "@weekly", "@monthly", "@every 4h", "0 12 * * *" ], free_text: true
Expand Down
Loading