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

Support {...} interpolation in body #3514

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

Support {...} interpolation in body #3514

wants to merge 8 commits into from

Conversation

josevalim
Copy link
Member

@josevalim josevalim commented Nov 18, 2024

The first commit has the code changes. The second commit runs mix format to migrate the codebase, so you can see if you prefer the new style or not.

cc @SteffenDE @chrismccord @connorlay @msaraiva

lib/phoenix_component.ex Outdated Show resolved Hide resolved
lib/phoenix_component.ex Outdated Show resolved Hide resolved
lib/phoenix_live_view/html_formatter.ex Outdated Show resolved Hide resolved
@@ -443,6 +444,14 @@ defmodule Phoenix.LiveView.TagEngine do
|> update_subengine(:handle_expr, [marker, expr])
end

defp handle_token({:body_expr, value, %{line: line, column: column}}, state) do
quoted = Code.string_to_quoted!(value, line: line, column: column, file: state.file)
Copy link
Contributor

Choose a reason for hiding this comment

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

For this template:

{if true do}

we currently receive this error:

** (TokenMissingError) token missing on iex:5:11:
    error: missing terminator: end5if true do
    │         │ └ missing closing delimiter (expected "end")
    │         └ unclosed delimiter
    │
    └─ iex:5:11

which is great but maybe there's still room to improve?

If we changed Code.string_to_quoted! to Code.string_to_quoted we could maybe improve the error message, telling people to use :if or <%= ... %>. The error tuple is quite expressive:

iex> Code.string_to_quoted("if true do")
{:error,
 {[
    opening_delimiter: :do,
    expected_delimiter: :end,
    line: 1,
    column: 9,
    end_line: 1,
    end_column: 11
  ], "missing terminator: end", ""}}

and I see it's like this since Elixir 1.16. Let me know if you'd like to pursue this and I'm happy to send a patch.

guides/server/live-layouts.md Show resolved Hide resolved
lib/phoenix_component.ex Show resolved Hide resolved
lib/phoenix_component.ex Show resolved Hide resolved
@SteffenDE SteffenDE added this to the v1.0 milestone Nov 18, 2024

<%!-- Phoenix.Component.upload_errors/1 returns a list of error atoms --%>
Copy link
Contributor

Choose a reason for hiding this comment

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

if the EEx syntax is going away it seem odd to keep it around just for comment tags. Why not a {% %} style?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not going away though, it is not possible to do {if ... do}, {for ... do}, etc. (Not that people should reach for those given :if and :for exists.) We can't use {...} interpolation inside <script> and <style> either and for that <%= ... %> is the way to go.

Copy link
Member Author

Choose a reason for hiding this comment

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

The EEx syntax is not going away per se because it is still needed in some situations (such as inside script, style, and template, as escaping { in there would be a pain) but I agree we should probably consider introducing something specific for comments later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

The <%!-- --%> comment syntax was inspired by Surface's {!-- --}. If we're moving to curly braces in the HTML, I'd vote we use the curly comments too.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for {!-- --} style

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.

6 participants