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

Reserve { outside of attributes #3498

Closed

Conversation

wojtekmach
Copy link
Contributor

Before this patch, this was valid:

<div id={id}>{content}</div>

and the text {content} is rendered as is, without interpolation. I'd like to reserve this syntax for later and for now raise an error:

iex> import Phoenix.Component, only: [sigil_H: 2]
iex> assigns = %{}; ~H"<div id={id}>{content}</div>"
** (Phoenix.LiveView.Tokenizer.ParseError) iex:3:14: `{` outside of attributes is reserved, use `\\{` to escape
  |
1 | <div id={id}>{content}</div>
    (phoenix_live_view 1.0.0-rc.7) lib/phoenix_live_view/tokenizer.ex:732: Phoenix.LiveView.Tokenizer.raise_syntax_error!/3
    (phoenix_live_view 1.0.0-rc.7) lib/phoenix_live_view/tag_engine.ex:295: Phoenix.LiveView.TagEngine.handle_text/3
    (eex 1.18.0-dev) lib/eex/compiler.ex:331: EEx.Compiler.generate_buffer/4
    (phoenix_live_view 1.0.0-rc.7) expanding macro: Phoenix.Component.sigil_H/2
    iex:2: (file)

If someone needs to use {, there's a way to escape it using \\{:

iex> assigns = %{}; ~H"<div id={:foo}>\\{content}</div>".static
["<div", ">\\{content}</div>"]

The idea is to eventually use the same interpolation syntax across the board. And when that happens, deprecate <%= %> in favour of { }. Supporting new syntax as well as deprecating old syntax is a massive change because of existing Phoenix projects, syntax highlighters, etc. I think we should be able to automate moving from <% %> to { } using tools like https://github.com/adobe/elixir-styler.

Before this patch, this was valid:

    <div id={id}>{content}</div>

and the text `{content}` is rendered _as is_, without
interpolation. I'd like to reserve this syntax for later
and for now raise an error:

    iex> import Phoenix.Component, only: [sigil_H: 2]
    iex> assigns = %{}; ~H"<div id={id}>{content}</div>"
    ** (Phoenix.LiveView.Tokenizer.ParseError) iex:3:14: `{` outside of attributes is reserved, use `\\{` to escape
      |
    1 | <div id={id}>{content}</div>
        (phoenix_live_view 1.0.0-rc.7) lib/phoenix_live_view/tokenizer.ex:732: Phoenix.LiveView.Tokenizer.raise_syntax_error!/3
        (phoenix_live_view 1.0.0-rc.7) lib/phoenix_live_view/tag_engine.ex:295: Phoenix.LiveView.TagEngine.handle_text/3
        (eex 1.18.0-dev) lib/eex/compiler.ex:331: EEx.Compiler.generate_buffer/4
        (phoenix_live_view 1.0.0-rc.7) expanding macro: Phoenix.Component.sigil_H/2
        iex:2: (file)

If someone needs to use `{`, there's a way to escape it using `\\{`:

    iex> assigns = %{}; ~H"<div id={:foo}>\\{content}</div>".static
    ["<div", ">\\{content}</div>"]

The idea is to eventually use the same interpolation syntax across the
board. And when that happens, deprecate `<%= %>` in favour of `{ }`.
Supporting new syntax as well as deprecating old syntax is a massive
change because of existing Phoenix projects, syntax highlighters, etc.
I think we should be able to automate moving from `<% %>` to `{ }`
using tools like https://github.com/adobe/elixir-styler.
@wojtekmach
Copy link
Contributor Author

cc @msaraiva

@wojtekmach
Copy link
Contributor Author

I’ll soon send and update that allows using { without escaping inside <style> and <script> tags.

@wojtekmach
Copy link
Contributor Author

I didn't have to change any code for <script> and <style>, I just added tests.

Looks like actually implementing interpolations with {} will be tricky. I thought we could easily reuse the EEx tokenizer but I think we'd have to either first teach it to support { or start from scratch. In either case it's a big undertaking. That being said, I still think it's good to reserve this now and give us flexibility in the future.

@msaraiva
Copy link
Contributor

Hey @wojtekmach!

Looks like actually implementing interpolations with {} will be tricky.

Yes, it will be tricky and probably require a major refactor in the EEx engine. Unless, of course, we can finally cut that umbilical cord and make HEEx completely independent from EEx, which would be absolutely gorgeous, IMO :)

Question: would partial code injection using <% %> still be possible? Is the goal to just replace <%= %> with { }?

@wojtekmach
Copy link
Contributor Author

I'd personally rather not but I think it is much more practical and pragmatic to support <%, <%=, etc basically forever. We need these inside <script> and <style> tags if we want to have interpolations as we can’t really use {.

@msaraiva
Copy link
Contributor

I removed support for interpolation inside <script> and <style> in Surface a long long time ago. Never looked back. The cost of potentially breaking a bunch of tools like linters, formatters, generators, etc. for the sake of allowing this bad practice, it's just not worth it, IMO. Most of those JS/CSS tools rely on parsable/correct code, which is hard to achieve when you start injecting alien code in it. Anyway, it's always about choices :)

@@ -155,6 +155,18 @@ defmodule Phoenix.LiveView.Tokenizer do
handle_tag_open(rest, line, column + 1, text_to_acc, %{state | context: []})
end

defp handle_text("{" <> rest, line, column, [?\\ | buffer], acc, state) do
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should allow escapes. Because if \{ means {, then how do I write \{? We would need to support \\{ and so on. We should probably follow JSX here. If you want to write {, you can either write { "{" }, &#123; or &lbrace;.

@josevalim
Copy link
Member

Closing in favor of #3514.

@josevalim josevalim closed this Nov 18, 2024
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.

3 participants