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 for Phoenix 1.6 & HEEx #92

Open
2 tasks
tensiondriven opened this issue Sep 1, 2021 · 18 comments
Open
2 tasks

Support for Phoenix 1.6 & HEEx #92

tensiondriven opened this issue Sep 1, 2021 · 18 comments

Comments

@tensiondriven
Copy link
Contributor

Phoenix 0.16 (and LiveView 0.16) introduce HEEx, which is a huge step forward, enabling semantic HTML parsing/validation.

I depend on phoenix_slime and am hopeful that the library can be updated to support HEEx. The main issues I see are:

  • Support { } syntax for attributes
  • Support dot-prefix for component functions (such as .form)

Since HAML/Slim/Slime reserve the dot for classes, it seems that this new convention in HEEx may be directly incompatible with Slim/Slime as we've known it. Hopefully there's some way to support it.

For anyone close to the code in this library, do you have a sense of what it would take to upgrade phoenix_slime to support the latest incarnations of Phoenix/LiveView/HEEx?

@thepeoplesbourgeois
Copy link

thepeoplesbourgeois commented Sep 4, 2021

Not a code owner, or really even a contributor, but I do have...

✨✨UNSOLICITED✨OPINIONS✨✨

about this.

First off, the { } attribute syntax seems to exist in order to allow HEEx to validate the structure of the HTML it will be writing. As of now, the Slime compiler renders its attribute values explicitly to the EEx wrapped variations:

  # /lib/slime/compiler.ex:98..108
  defp render_attribute_code(name, _content, quoted, _) when is_list(quoted) do
    quoted |> Enum.map_join(" ", &Kernel.to_string/1) |> (& ~s[ #{name}="#{&1}"]).()
  end
  defp render_attribute_code(name, _content, quoted, :eex) when is_binary(quoted), do: ~s[ #{name}="#{quoted}"]
  defp render_attribute_code(name, _content, quoted, _) when is_binary(quoted), do: ~s[ #{name}="<%= {:safe, "#{quoted}"} %>"]

  # NOTE: string with interpolation or strings concatination
  defp render_attribute_code(name, content, {op, _, _}, safe) when op in [:<<>>, :<>] do
    value = if safe == :eex, do: content, else: "{:safe, #{content}}"
    ~s[ #{name}="<%= #{value} %>"]
  end

I may be woefully uninformed on this, but the change to accommodate HEEx's variable syntax seems like it might just be as simple as replacing the <%= and %> with { and }, respectively, in and omitting the {:safe, "#{quoted}"} tuples altogether when rendering to HEEx?


The component name problem is a trickier one, as it will need a clever solution, and clever is the enemy of clean, but I do think the only way of working through it is going to be to add another symbol (or delimiter) to Slime's PEG. In fact, the symbol may be redundant, as templates would still need to incorporate delimiters in order to know when MyApp.Component.function ends, and mycomponent.element.class-names begin.

... Maybe... in a tongue-in-cheek reversal... Slime could delimit component names like...

main.container
  article
    {MyApp.Component.function}.mycomponent.element.class-names[and=:then come=~w[the attributes]]

? Maybe...? Or does the curly braces' similarity to the standard #{} interpolation delimiter make the distinction too confusing?

The other idea I had would be to adopt something akin to the struct declaration (which would be somewhat apropos, given the overall similarity between a component with properties and a struct with fields), and denote a delimited component name with a % (or some other symbol available to the PEG),

main.container
  %{MyApp.Component.function}.mycomponent.class-names[and=:then come=~w[the attributes]]
    section.foo.bar

/ or maybe even literally

main.container
  %MyApp.Component.function{and: :then, come: ~w[the attributes]}.mycomponent.class-names
    section.foo.bar

@paradox460
Copy link

paradox460 commented Sep 27, 2021

I may be woefully uninformed on this, but the change to accommodate HEEx's variable syntax seems like it might just be as simple as replacing the <%= and %> with { and }, respectively, in and omitting the {:safe, "#{quoted}"} tuples altogether when rendering to HEEx?

From a surface level, that appears to be a good take. However some deep recess of my mind warns me that there is HTML escaping magic going on around <%= and friends, and a 1:1 switch might have impact.


I really like the literal struct-like implementation. It feels elixiry, and I'm having a hard time mentally poking holes in it. Shouldn't be too difficult to get the PEG to support either.

Amusingly, Slim's precursor, HAML, used % to deliniate plain tags, like this:

%html
  %head
    %title Foo

Slim's greatest idea was tossing that useless marker:

html
  head
    title Foo 

@stevensonmt
Copy link

Total noob here but saw this issue on reddit and thought it was interesting. I had an idea for how to approach the dot syntax collision. Essentially you would have two paths when parsing an element preceded by a dot. First you need a list of all modules in the project, which can be generated with :application.get_key(:my_app, :modules) where my_app is the top-level name of the project. Then use module.__info__(:functions) to generate a keyword list of functions defined in each module with function name as the key and arity as the value. When the HEEX slime engine encounters a dot element, it would search that keyword list for a matching function with matching arity and assigns. If no such function is found then treat it as a CSS class syntax.
Does this seem at all workable or am I missing a key component somewhere?

@tensiondriven
Copy link
Contributor Author

@stevensonmt Sounds reasonable to me. Not sure if that would have implications with compilation vs runtime, since i don't know the details of the internals very well, but that sounds like a reasonable approach.

Another thought I had based on what I just read on the phoenix_live_view changelog for 0.17, copied below:

Add <.live_component module={FormComponent} id="form" />

If this is true, could we completely forgo supporting the .Module syntax by adding a special case for lines starting with live_component and simply render out HEEx using this format? Users would have to rely on this "long hand" syntax for referencing components, but it wouldn't require any changes to how dots are used/parsed. That would at least get the dep into a state that's usable/compatible with the latest liveview.

Ref: https://github.com/phoenixframework/phoenix_live_view/blob/master/CHANGELOG.md#enhancements

@paradox460
Copy link

@stevensonmt while thats entirely possible, it does open the door to more complexities, namely, the need to keep (and update on HMR) a list of all the modules in the codebase, and a scan when compiling slim files into render functions.

It also has no clean way of avoiding collisions. What if you have a Header component, for example.

Making slime respect Capitalized names as modules would do the trick, but imo that cutesy sort of stuff becomes a foot-gun down the road.

@tensiondriven
Copy link
Contributor Author

I'm all for keeping it simple. I'd love to get something going that I could start to work with, test, and use, even if it was pretty verbose (as in using %{MyApp.Component.function} or a live_component directive)

@paradox460 Any thoughts about copying 0.17's convention of using a live_component prefix?

@paradox460
Copy link

@tensiondriven it does not seem very slim-like, but it does get stuff working faster, so there's the tradeoff. IMO the %{} syntax is the most elegant

@tensiondriven
Copy link
Contributor Author

I dug into this pretty deeply - To implement HEEx name/attribute awareness, the changes to the library itself aren't that significant, its more how the engines are wired up. Here's what I found so far (posting for my own learning and recollection later, and possibly the benefit of others)

In Phoenix app,

config.exs configures engines:

config :phoenix, :template_engines,
  slim: PhoenixSlime.Engine,
  slime: PhoenixSlime.Engine,
  sleex: PhoenixSlime.LiveViewEngine

PhoenixSlime.LiveViewEngine implements:

  @behaviour Phoenix.Template.Engine

  @doc """
  Precompiles the String file_path into a function definition
  """
  def compile(path, _name) do
    path
    |> read!()
    |> EEx.compile_string(engine: Phoenix.LiveView.Engine, file: path, line: 1)
  end

Phoenix.LiveView.Engine parses EEx with change tracking.

phoenix_slime needs to implement a new module, Phoenix.LiveViewHTMLEngine which calls EEx.compile_string with Phoenix.LiveView.HTMLEngine instead of Phoenix.LiveView.Engine

(From the docs, Phoenix.LiveView.HTMLEngine is the HTMLEngine that powers .heex templates and the ~H sigil)

The new Phoenix.LiveViewHTMLEngine module needs to branch the logic in Slime.Renderer.precompile to use { } instead of <%= %>, as @stevensonmt suggested.

This doesn't address how to support the dot-syntax for live_components yet, however 0.17 was just released which provides a live_component function. To support pure html aware live components, one should be able to call = live_component directly, though I'm not sure if calling = live_component from an EEx/Elixir expression would break change tracking between the parent template and the pure live component.

@tensiondriven
Copy link
Contributor Author

I discovered something interesting and problematic. This function:

  defp render_attribute_code(name, content, _, safe) do
    value = if safe == :eex, do: "slim__v", else: "{:safe, slim__v}"
  
    """
    <% slim__k = "#{name}"; slim__v = Slime.Compiler.hide_dialyzer_spec(#{content}) %>\
    <%= if slim__v do %> <%= slim__k %><%= unless slim__v == true do %>\
    ="<%= #{value} %>"<% end %><% end %>\
    """
  end

is responsible for taking a name/value pair and producing valid EEx/HEEx, I think. It does so by producing some intermediate inline elixir, escaped with <%= %>. This was fine in the land of EEx but in HEEx, anything in an attribute value must be something change-trackable. So, whatever incantation that is being done in that escaped elixir causes the HEEx engine to barf:

expected closing `>` or `/>`

Make sure the tag is properly closed. This may happen if there
is an EEx interpolation inside a tag, which is not supported.
For instance, instead of

    <div id="<%= @id %>">Content</div>

do

    <div id={@id}>Content</div>

If @id is nil or false, then no attribute is sent at all.

Inside {...} you can place any Elixir expression. If you want
to interpolate in the middle of an attribute value, instead of

    <a class="foo bar <%= @class %>">Text</a>

you can pass an Elixir string with interpolation:

    <a class={"foo bar #{@class}"}>Text</a>

    lib/phoenix_live_view/html_tokenizer.ex:277: Phoenix.LiveView.HTMLTokenizer.handle_maybe_tag_open_end/5
    lib/phoenix_live_view/html_engine.ex:121: Phoenix.LiveView.HTMLEngine.handle_text/3
    (eex 1.12.2) lib/eex/compiler.ex:48: EEx.Compiler.generate_buffer/4
    (phoenix_view 1.0.0) lib/phoenix/template.ex:371: Phoenix.Template.compile/3
    (phoenix_view 1.0.0) lib/phoenix/template.ex:165: anonymous fn/4 in Phoenix.Template."MACRO-__before_compile__"/2
    (elixir 1.12.2) lib/enum.ex:2385: Enum."-reduce/3-lists^foldl/2-0-"/3
    (phoenix_view 1.0.0) expanding macro: Phoenix.Template.__before_compile__/1
    test/phoenix_slime_heex_test.exs:7: PhoenixSlimeHeexTest.MyApp.PageView (module)
    (elixir 1.12.2) lib/kernel/parallel_compiler.ex:428: Kernel.ParallelCompiler.require_file/2
    (elixir 1.12.2) lib/kernel/parallel_compiler.ex:321: anonymous fn/4 in Kernel.ParallelCompiler.spawn_workers/7

I'm not sure why this has to be represented as inline EEx here; perhaps because the function with represents the template is generated at compile time, and that inline EEx needs to be evaluated at runtime? That's all I can think of so far.

@tensiondriven
Copy link
Contributor Author

I've got a working branch! It requires changes to both slime and phoenix_slime. The changes are actually pretty trivial on both sides (assuming I didn't do something horribly wrong).

I needed to branch the logic in the Slime.Compiler and couldn't find an elegant way, so for now duplicated the whole module. I'll review it with a comrade and clean it up, then push a branch of slime. I'm sure there will be some feedback.

PS the new file extension for a slime - heex file is .sheex.

@tensiondriven
Copy link
Contributor Author

I have a branch running that successfully generates HEEx which is then parsed by to produce HTML. I would love some feedback on the approach. The work is done in the slime repo, not phoenix_slime, so I'd love to move the conversation over there:

slime-lang/slime#166

@thepeoplesbourgeois
Copy link

PS the new file extension for a slime - heex file is .sheex.

trés chic

@silverdr
Copy link

This issue seems to be open for some time now so the question would be how does the LiveView support for Phoenix 1.6+ looks as of now?

@tensiondriven
Copy link
Contributor Author

It's working great, I'm using it in production and haven't run into any issues.

Currently it doesn't support a sigil, which as far as I know is the only parity difference between the existing dep and this. At this point, I think we're waiting for @doomspork to merge and cut a release. The sigil will be addressed later.

As far as I know, there hasn't been much testing apart from me.

I'd love some testing help/feedback, if you'd like to give it a go:

[
   {:phoenix_slime, github: "tensiondriven/phoenix_slime"},
]

@silverdr
Copy link

silverdr commented Mar 24, 2022

@tensiondriven - thank you for your response! So basically with your branch one adds sheex: PhoenixSlime.LiveViewEngine to the config and there it goes, right? BTW - I think I should move to slime-lang/slime#166 so if you respond you can do it already there.

@rsieb
Copy link

rsieb commented May 11, 2022

Hi @tensiondriven I'm crossposting tensiondriven/phoenix_slime_test#1 (comment) here as it may be more relevant to this thread. Tldr; does the fix not support phx.gen.html.slime for templates yet?

@simonallfrey
Copy link

Hi @tensiondriven, did you have any luck contacting @thepeoplesbourgeois to reboot this PR?

@andrewfader
Copy link

What's the story? Anyone looking at this anymore?

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

No branches or pull requests

8 participants