Adoption of used_input?/2 for LiveView 1.0.0?

carlgleisner commented 1 week ago

I love what you've done with this terrific component, so grateful ❤️ Amazing work.

I'm humbly opening this issue in the hope of tracking a discussion regarding possible adoption of the upcoming used_input?/2 API for conditionally showing error messages depending on whether it is time to show them or not.

https://hexdocs.pm/phoenix_live_view/1.0.0-rc.7/changelog.html#backwards-incompatible-changes-for-1-0

LiveSelect and LiveView versions

live_select: 1.4.2
phoenix_live_view: 1.0.0-rc.7

Describe the bug When passing the field.errors to the component after first having checked whether the field is considered to have been used with Phoenix.Component.used_input?/2 function, the error is shown since the field won't match what is expected by LiveView's new logic.

Please consider the following component from the README adapted to operate like the new Phoenix LiveView 1.0 core components for inputs.

attr :id, :any
attr :label, :string

attr :update_min_len, :integer

attr :field, Phoenix.HTML.FormField
attr :errors, :list, default: []

def live_select(%{field: %Phoenix.HTML.FormField{} = field} = assigns) do
  # This is practical for debugging since the `_unused*` parameters determine the logic for `used_input?/2`
  field.form.params |> dbg()

  errors =
    if Phoenix.Component.used_input?(field),
      do: field.errors,
      else: []

  assigns =
    assigns
    |> assign(:errors, Enum.map(errors, &translate_error(&1)))
    |> assign(:live_select_opts, assigns_to_attributes(assigns, [:errors, :label]))

  ~H"""
  <div>
    <.label for={@field.id}><%= @label %></.label>
    <LiveSelect.live_select
      field={@field}
      text_input_class={[
        "mt-2 block w-full rounded-lg border-zinc-300 py-[7px] px-[11px]",
        "text-zinc-900 focus:outline-none focus:ring-4 sm:text-sm sm:leading-6",
        "border-zinc-300 focus:border-zinc-400 focus:ring-zinc-800/5",
        @errors != [] && "border-rose-400 focus:border-rose-400 focus:ring-rose-400/10"
      ]}
      {@live_select_opts}
    />

    <.error :for={msg <- @errors}><%= msg %></.error>
  </div>
  """
end

In a simple form with the fields name and manager the LiveSelect would have a hidden input with the field manager_text_input.

Looking at the params passed in with field.form.params, the first validation of the form after entering text into the name input would result in the following params.

field.form.params #=> %{
  "_unused_manager_text_input" => "",
  "manager" => "",
  "manager_text_input" => "",
  "name" => "a",
}

Since calling unused_input?/2 with the field as it is would check for any parameter _unused_manager but not being able to find any such parameter, the LiveSelect is incorrectly considered to be used even though the LiveSelect field manager_text_input actually has not been used yet.

For reference, see the implementation of used_input?/2:

https://github.com/phoenixframework/phoenix_live_view/blob/b59bede3fcec6995f1d5876a520af8badc4bb7fb/lib/phoenix_component.ex#L1605

def used_input?(%Phoenix.HTML.FormField{field: field, form: form}) do
  used_param?(form.params, field)
end

defp used_param?(_params, "_unused_" <> _), do: false

defp used_param?(params, field) do
  field_str = "#{field}"
  unused_field_str = "_unused_#{field}"

  case params do
    %{^field_str => _, ^unused_field_str => _} -> false
    %{^field_str => %{} = nested} -> Enum.any?(Map.keys(nested), &used_param?(nested, &1))
    %{^field_str => _val} -> true
    %{} -> false
  end
end

Expected behavior Technically not expected behaviour since the issue concerns a breaking change, but once adapted to the new LiveView interface the expected behaviour would be:

Interacting with the field in a way that would consider it as "used" shows any present errors
Editing another input field does not result in the error being shown

Actual behavior Editing another field causes the LiveSelect field to immediately be considered as used, thus rendering any error even though the time is not right for it to be shown.

Screenshots None available.

Browsers Edge

Issue Repo I'd be happy to provide one if requested.

Additional context I've been looking into composite fields before but I believe that situation is different since there is no hidden field in that scenario. But if it helps, here is a rather lengthy issue with a final comment form Mr. McCord:

https://github.com/phoenixframework/phoenix_live_view/issues/2968#issuecomment-2389256516

carlgleisner commented 1 week ago

I'm doing this now:

key_to_drop = "_unused_" <> Atom.to_string(field.field) <> "_text_input"
key_to_replace = "_unused_" <> Atom.to_string(field.field)

maybe_patched_params =
  if Map.has_key?(field.form.params, key_to_drop) do
    field.form.params
    |> Map.drop([key_to_drop])
    |> Map.put(key_to_replace, "")
  else
    field.form.params
  end

form = field.form
form = %{form | params: maybe_patched_params}
field = %{field | form: form}

Not my proudest moment but it works 😇 (as far as I can tell)

maxmarcon commented 1 week ago

Thanks for raising awareness of this issue. I haven't followed the latest LV development and had no clue of the existence of Phoenix.Component.used_input?/1.

Question: wouldn't this simplified version of your workaround also work?

text_input_field = %{field | field: String.to_atom("#{field.field}" <> "_text_input")}

errors =
    if Phoenix.Component.used_input?(text_input_field),
      do: field.errors,
      else: []

...

maxmarcon commented 1 week ago

Sorry I had closed this by mistake, I reopened it

carlgleisner commented 1 week ago

Question: wouldn't this simplified version of your workaround also work?

Yes indeed it works the same. I merely dropped the first attempt of a workaround while familiarising myself with the API. Yours is the elegant one.

But please note that it's not a fully functioning workaround since there will be cases where the fix won't be applied because – as far as I can tell – the component isn't being redrawn.

I'd be interested to learn the correct place for handling this. Composite inputs required a change upstream – maybe here too?

I'll share any insights once I gain them.

maxmarcon commented 1 week ago

But please note that it's not a fully functioning workaround since there will be cases where the fix won't be applied because – as far as I can tell – the component isn't being redrawn.

This is weird. Are you sure? Could you share a repository that shows this behavior?

carlgleisner commented 1 week ago

This is weird. Are you sure?

Yes I'm sure that it isn't being re-rendered and I've gone on to check that regular core component inputs in the same scenario are re-rendered.

So:

Submission of a blank form re-renders the LiveSelect and properly shows the errors with the discussed workaround in the function component wrapping the LiveSelect live component
Submission of a modified form does not re-render the LiveSelect and therefore the discussed workaround is never executed

I don't know man. I wish I could help out more but if one could just get the wrapping function component to re-render it seems like it would be all dandy.

Could you share a repository that shows this behavior?

I truly wish I was able to spin up a reproduction but I'm sadly unable to do so at this point due to having rather limited capacity at the moment and foreseeable future.

maxmarcon commented 1 week ago

Can you please try with the latest version that was published yesterday? (1.4.3). Thanks

carlgleisner commented 1 week ago

I already did since I saw that you released it.

I'll do some digging in LiveView and try to get back asap, but my asap may be many days or several weeks presently.

Thanks so much for getting back to me on this one 🙂

maxmarcon / live_select

Adoption of used_input?/2 for LiveView 1.0.0? #85