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

Send JS command values on phx-submit #3674

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jotaviobiondo
Copy link

@jotaviobiondo jotaviobiondo commented Feb 12, 2025

Currently, when setting the phx-submit with a JS.push/3 with the :value attribute set, it does not send this value to the server (only the phx-value-* are sent)

Example:

<form id="userForm" phx-submit={JS.push("submit", value: %{command_value: "some value"})}>
   ...
</form>

The %{command_value: "some value"} will not be present in the handle_event/3 params.

@jotaviobiondo jotaviobiondo marked this pull request as draft February 12, 2025 22:37
@jotaviobiondo jotaviobiondo marked this pull request as ready for review February 12, 2025 23:40

return params.toString()
}

let appendToUrlParams = (params, name, value) => {
if(Array.isArray(value)){
value.forEach((v) => appendToUrlParams(params, `${name}[]`, v))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'd need to add the index here, see https://hexdocs.pm/plug/Plug.Conn.Query.html

image

Copy link
Author

Choose a reason for hiding this comment

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

Hmm good point! But I think this way the decoding of simple arrays like [1,2,3] would not not what users expect.

So we need to handle the case where the value inside the arrays are maps/objects and I think there's 2 ways of doing this:

  1. Automatically handles this for the user, adding the index like in the Plug.Conn.Query docs suggests
  2. Warn (using console.error or something) the user about this, saying it's not allowed and let the user change his data to have the map with indexes instead of the array.

I'm more inclined to go with the 2nd option, because the 1st also would generate unexpected values in the server, even if this is documented. The 2nd one would fail immediately and the user can adapt their code to fix it and no unexpected behavior happens.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm I'm not sure. What if we just don't support nested values at all? I feel like the whole phx-value-* thing or JS.push with extra value is not really necessary with forms. In those cases, you can always add hidden inputs or, if the values are static values, just merge them in the event handler instead. So I'm thinking maybe we don't support this and properly document it?

Copy link
Author

Choose a reason for hiding this comment

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

It's another option for sure!

But I think there are use cases where this would be useful. For example, the thing I'm working on where I faced this unexpected behavior is a form inside a modal that receives parameters.

In my case, a modal would be opened via fronted with this.pushEvent from hook, passing something like %{selected_ids: [...]} to the event. Then this params are saved in the modal live component and would be passed together with the form phx-submit and in the handle event I would receive something like %{"selected_ids" => [...], "form" => %{...}}.

This is a very specific case and of course there are other ways to do it, but doing with JS.push/2 and passing a value seems the more natural way. Also, I think passing additional parameters to the submit event could be useful in other similar cases as well.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @jotaviobiondo,

I opened #3688 with an alternative implementation that does not require messing with the formData encoding. Let me know what you think :)

Copy link
Author

Choose a reason for hiding this comment

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

Nice! so instead of passing it together with the form params, you passed as the event metadata over the socket, I like it! it's simpler and probably more correct implementation :)

Added just a question in there https://github.com/phoenixframework/phoenix_live_view/pull/3688/files#r1970297748

But for me looks good!

SteffenDE added a commit that referenced this pull request Feb 23, 2025
Alternative implementation for #3674.

Co-Authored-By: João Otávio Biondo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants