JS.push doesn't accept :event props

Surface: v0.7.1
LiveView: v0.17.7
Elixir: v1.13.3

Try to pass an event prop to JS.push and it will crash on:

** (FunctionClauseError) no function clause matching in Phoenix.LiveView.JS.push/2
    (phoenix_live_view 0.17.7) lib/phoenix_live_view/js.ex:150: Phoenix.LiveView.JS.push(%{name: "clear_options_filter", target: :live_view}, [value: %{field: :cluster}])

Workaround

defmodule Surface.JS do
  alias Phoenix.LiveView.JS

  def push(event) when is_binary(event) do
    JS.push(event)
  end

  def push(event) when is_map(event) do
    push(%JS{}, event, [])
  end

  def push(event, opts) when is_binary(event) do
    JS.push(event, opts)
  end

  def push(event, opts) when is_map(event) and is_list(opts) do
    push(%JS{}, event, opts)
  end

  def push(%JS{} = js, event, opts) when is_binary(event) and is_list(opts) do
    JS.push(js, event, opts)
  end

  def push(%JS{} = js, %{name: name, target: :live_view}, opts) when is_list(opts) do
    JS.push(js, name, opts)
  end

  def push(%JS{} = js, %{name: name, target: target}, opts) when is_list(opts) do
    JS.push(js, name, Keyword.put(opts, :target, target))
  end
end

<button phx-click={Surface.JS.push(@some_event, value: %{field: @field})}>button</button>

Should we create a Surface.JS and override push?

@simonmcconnell

Hi @simonmcconnell!

Ideally, we should have :on-* accepting something like:


<button :on-click={@some_event, value: ..., loading: ...}>button</button>

which would already handle the default target and be equivalent to:

<button :on-click={JS.push(@some_event, value: ..., loading: ...)}>button</button>

And since now we finally have a way to represent an event with both, the name and the target, using the JS struct, we should probably deprecate support for %{name: name, target: target} and use the JS struct instead. I'll work on a PR for that.

I forgot to mention that currently, we only accept the target as option so value, loading or any other will not be handled if you try to pass them as above. For those cases, you need to use JS.push explicitly for now.

Hi marlus. When you say "as above", are you referring to your ideal future scenario?

<button :on-click={@some_event, value: ..., loading: ...}>button</button>?

@simonmcconnell

@simonmcconnell yes!

@simonmcconnell

Hey @simonmcconnell!

~~I was starting to play around with this and I noticed that it might not be a good idea to handle events that way.~~

The problem is that Phoenix's JS API does not allow setting any option like target or loading after we call JS.push. The first argument of JS.push must always be a string. If you try to pass a %JS{}, it will break with a no function clause matching in Phoenix.LiveView.JS.push/2.

I still think is valuable to add syntactic sugar for JS.push like:

<button :on-click={@some_event, value: ..., loading: ...}>button</button>

as it's the most common use for it, but the first argument (in this case, @some_event) will have to be a string, not a %JS{}.

So for cases like you described, where you need to set the options later, the best solution would be to change the type of the prop from :event from :string or maybe a :tuple like {event_as_string, opts} which you could handle later in your component, updating the opts you need before calling JS.push.

I would not recommend using it as :event and then handling %{name: ..., target: ...} yourself because, as I mentioned, we plan to deprecate it and eventually remove it.

~~If the JS API evolves and allow us to set options on an existing JS struct, then we could reconsider adding this feature.~~

~~I'm closing this for now. Feel free to reopen it in case you have any updates.~~

Thanks for taking a look.

I'm not sure I follow with respect to setting the options later. Doesn't my dodgy workaround set them when adding the push operation to the %JS{}?

JS.push takes a %JS{}, but it also requires an event string. Can't we collect the target from the event and pass it as an option to JS.push? If the first arg passed to :on-click is a string, we pass that straight to JS.push. And we accept the same opts in :on-click as JS.push?

But that doesn't really help for :on-click{JS.push(@event_prop, value: ...) |> JS.hide(...) |> JS.dispatch(...)}, which is why I thought we would need to provide our own JS module which delegates to LiveView's, except for push, so we can extract the event name and target from the event prop. Whether or not that is possible is a question I do not know the answer to 😄

LiveView source:

  def push(%JS{} = js, event, opts) when is_binary(event) and is_list(opts) do
    opts =
      opts
      |> validate_keys(:push, [:target, :loading, :page_loading, :value])
      |> put_target()
      |> put_value()


    put_op(js, "push", Enum.into(opts, %{event: event}))
  end

@simonmcconnell

@simonmcconnell I totally misunderstood the whole thing. Thanks for persisting on the issue. I believe we should work on replacing %{name: ..., target: ...} with a JS struct as soon as possible as it would not only fix the issue but it would also make the API more flexible allowing users to update events as they wish without introducing a new custom API.

I'm reopening it. Thanks again!

No problem. Let me know if you want a hand with it.

Navigation Menu

Repository navigation

JS.push doesn't accept :event props #573

Workaround

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

JS.push doesn't accept :event props #573

Description

Workaround

Activity

msaraiva commented on Mar 7, 2022

msaraiva commented on Mar 7, 2022

simonmcconnell commented on Mar 7, 2022

msaraiva commented on Mar 13, 2022

msaraiva commented on Mar 13, 2022

simonmcconnell commented on Mar 14, 2022

msaraiva commented on Mar 14, 2022

simonmcconnell commented on Mar 14, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions