Build form data using Plug.Conn.Query (remove DeepMerge) #94
Conversation
| def build_data(data) when is_list(data) do | ||
| data | ||
| |> Enum.map_join("&", fn {key, value} -> | ||
| "#{URI.encode_www_form(key)}=#{if(value, do: URI.encode_www_form(value))}" |
There was a problem hiding this comment.
It would be nice to skip the encode/decode cycle.
But that's the only public interfacePlug.Conn.Query seems to offer.
On the other hand, it seems reasonable to use a standardized format (application/x-www-form-urlencoded) as the interface.
|
This PR does sound like a good idea to me. I tried supporting multiple inputs with ending with |
There was a problem hiding this comment.
Thanks for opening this PR! 🥳
I was out for a little while, so I didn't have a chance to look at it til now.
I love this work. In my mind, this was the next logical step. we dealt with deep merging of data because of how we originally supported map data being passed for forms with fill_form.
But since we've moved to using labels, it makes sense we keep the names flat as much as possible and encode/decode that stuff only when we need to send it to the server. So, I'm glad to see this coming through! 👍
I had one question about a test that was modified (see the comment on that question), and I want to play around locally with this branch a little more before merging. But I think this is almost ready to go.
Thanks again!
| |> Enum.reduce(value, fn key, acc -> | ||
| %{key => acc} | ||
| end) | ||
| end |
| @@ -36,7 +36,6 @@ defmodule PhoenixTest.MixProject do | |||
| defp deps do | |||
| [ | |||
| {:ex_doc, "~> 0.31", only: :dev, runtime: false}, | |||
| {:deep_merge, "~> 1.0"}, | |||
| @@ -451,11 +451,11 @@ defmodule PhoenixTest.StaticTest do | |||
| |> assert_has("#form-data", text: "race_2: [elf,dwarf]") | |||
| end | |||
|
|
|||
| test "honors empty default for multi select", %{conn: conn} do | |||
| test "contains no data for empty multi select", %{conn: conn} do | |||
There was a problem hiding this comment.
I think the "honoring" of an empty multi select was intentionally introduced in f146186. Can you confirm that's no longer needed? Or will this solution equivalent?
There was a problem hiding this comment.
From what I see, f146186 was mainly about accepting the boolean multiple attribute (and not required multiple="multiple".
I think this test, added in the same PR, was slightly incorrect.
- A HTML select (not multiple) submits the first option by default, even without user input.
- A HTML multiple select submits nothing by default.
Quick demonstration: Add multiple to the Try it select tag: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/select
In a real world example, you would probably add a hidden input with empty value before the multi select.
To ensure the value can be cleared. Much like for checkboxes.
Best if @soundmonster confirms this test should be changed.
There was a problem hiding this comment.
Yes can confirm that clearing multi-select values only works if an explicitly empty value is hidden next to it, just like with checkboxes 👍
There was a problem hiding this comment.
So you agree it is correct to change the test? @soundmonster
There was a problem hiding this comment.
Then let's get this merged @germsvel , wdyt?
There was a problem hiding this comment.
Nice! 🎉 Thanks for checking on this.
Fixes #77
Replaces #79, #90, #93
Goal
Ensure same form handling semantics as Phoenix controllers and LiveView.
Details
Track form data as a flat list of
{name, value}tuples.This can be read straight from the DOM and needs no further conversions.
Example:
Defer transforming that flat list into a nested data structure: When submitting form or evaluating other LiveLive
phx-*callbacks.Use Plug.Conn.Query.decode/4 to transform flat list into nested data structure. Reason: Phoenix Controllers and LiveView do the same (see #77 (comment)).