Support for Phoenix 1.6 & HEEx

[tensiondriven]

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

I adore and depend on 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?

Crossposted to slime-lang/phoenix_slime#92

[tensiondriven]

I have a working branch here, with the exception of a few tests. I would love to get some feedback on the approach.

I had to fork the logic used in the Compiler module because the output of Compiler is different when targetting .eex vs .heex. I didn't want to duplicate the logic, so end up using a metaprogramming approach. I'm not sure its the right approach because of a bug I'm seeing when running the integration tests.

Here's a PR I made against my own fork:

https://github.com/tensiondriven/slime_heex/pull/1/files

I'm a little new to doing PR's on OSS, so any advice would be welcome. cc @doomspork @topherhunt

[johns10]

I'm jazzy jazzed about this @tensiondriven. Thanks for the workup. Really looking forward to this hitting the release. Slim is god's gift to templateers, and you are it's prophet.

[tensiondriven]

Thanks @johns10 - getting this kind of feedback really helps motivate me.

Since posting that PR, I revisited the methodology and now have passing tests on a working branch that doesn't use the two Compiler modules. I will do my best to get something for others to test soon. It's a little tricky trying to support both EEx and HEEx, and also working with both slime and phoenix_slime but I think the hard parts are done. There are probably some problems with corner cases that are lurking in there, and possibly whole features that I just downright missed.

Anyway, more soon. Thanks again for the support!

[tensiondriven]

I've added HEEx support to the slime and phoenix_slime repos!

Currently my forks are available at:

https://github.com/tensiondriven/slime
https://github.com/tensiondriven/phoenix_slime

Here is a simple demo app that uses the above repos:

https://github.com/tensiondriven/phoenix_slime_test

For now, you can use the above repos to generate HEEx from slime.

I haven't submitted a request to merge my branch into master/main in the slime repo, but I think that may be the next step.

[doomspork]

This is amazing @tensiondriven! Thanks so much for taking the initiative on this 🎉

If you want to open a PR for these changes we can get them reviewed, provide any feedback, and then merge these in and release 🚀

[tensiondriven]

Alrighty, here it is!

#168

I'm sure there are some issues lurking, partially because of the complexity of having to use both slime and phoenix_slime, and the interactions with the EEx/Liveview engine. I'm still a bit perplexed by the boundary between Phoenix and Phoenix Live View, Phoenix.LiveView.HTMLEngine and Phoenix.LiveView.Engine.

I haven't done many PR's on OSS projects. I'll do my best to integrate any feedback on the PR. I'm not sure on the process for actually getting this merged in, so for now I put "[do not merge]" in the PR title, so we can integrate feedback and give it some time to bake. It seems we'll also have to coordinate review and merge of tensiondriven/phoenix_slime once this PR looks good to merge.

For now, if anyone on this thread is anxious to slime their liveviews, you can add the candidate repos to your project:

https://github.com/tensiondriven/slime
https://github.com/tensiondriven/phoenix_slime

Note you may need to change your mix.exs file in phoenix_slime as the dependency on slime is referencing a local repo.

[johns10]

When I started my current project that we weren't supporting Phoenix 1.6 yet. I'm really pleased about this workup. Can't wait!

[tensiondriven]

@doomspork I just updated the PR with a couple little changes (caught some extra logging and such) and removed "DO NOT MERGE" from the title.

Is there anything else I can do to move this forward? Can we get the Assigned To field set for the PR maybe?

[tomfarm]

Hi @tensiondriven,

thanks a lot for you work! I'm testing your pull request at the moment and it works quite well!

I'm wondering if slime supports something like Slime-Heex-Sigils. Currently there are only ~L and ~l (as far as I know) and both render as plain text when used inside the liveview render(assigns) function, but it may be that I'm missing something there.

As I'm not that deep inside the slime/phoenix-slime source code, could something like this work? I mean like correctly work the whole heex pipeline?

It would be nice though if Elixir supported sigils with multiple characters, ~S is already in use and we are running out of characters 😄

phoenix_slime/lib/phoenix_slime.ex

defmacro sigil_M({:<<>>, meta, [expr]}, []) do
  options = [
    engine: Phoenix.LiveView.HTMLEngine,
    file: __CALLER__.file,
    line: __CALLER__.line + 1,
    module: __CALLER__.module,
    indentation: meta[:indentation] || 0
  ]   

  Slime.Renderer.precompile_heex(expr)
  |> EEx.compile_string(options)
end

[tensiondriven]

Yes, since phoenix_slime already has support for ~L, I don't see why we couldn't add a sigil for slime-heex.

I also wonder if it makes sense to deprecate or remove the liveview-eex (leex) functionality)... Since heex came out, I'm really not clear on the line between HEEx and LiveView. But I guess thats another issue.

Would it make sense to take the addition of sigil support as a separate issue, and continue to consider this a candidate for merging, so we don't slow things down more? It may be weeks before I'm able to revisit this to implement the sigil (though I also imagine its not more than a couple of hours to add support for it).

[tomfarm]

You are right, moving the sigils to a separate issue makes more sense. 👍
I've opened a new issue #169

[tensiondriven]

@tomfarm Great! Do you know who would give the LGTM on this and pull the trigger on the merge?

[tomfarm]

@tensiondriven I'm sorry, I don't know.

[sheharyarn]

Hey @doomspork, it would be great if we can get your stamp on the linked PR and get a new version published with the changes.