-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add support for HEEx. #168
base: master
Are you sure you want to change the base?
Conversation
Co-Authored-By: Topher Hunt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I don't have permission to commit this, but I was thinking of using your branch, so I thought I'd review your code and offer some comments.
Thanks for writing this fork and I hope some of these comments are useful!
|
||
eex = Renderer.precompile(source) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eex = Renderer.precompile(source) | |
eex = source |> Renderer.precompile() |
Removing a formatting change to simplify the diff.
""" | ||
defmacro function_from_file(kind, name, file, args \\ [], opts \\ []) do | ||
quote bind_quoted: binding() do | ||
require EEx | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing a formatting change to simplify the diff.
|
||
alias Slime.TemplateSyntaxError | ||
|
||
@eex_delimiters {"#" <> "{", "}"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eex_delimiters {"#" <> "{", "}"} | |
@eex_delimiters {"\#{", "}"} |
Any reason not to write this way?
def eex_delimiters, do: @eex_delimiters | ||
def heex_delimiters, do: @heex_delimiters | ||
|
||
def compile([], _delimiters), do: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def compile([], _delimiters), do: "" | |
def compile(x), do: compile(x, @eex_delimeters) | |
def compile([], _delimiters), do: "" |
Might (or might not!) be good to avoid breaking the public API of the module by preserving compile/1
.
If this is done, all the changes to test/compiler_test.exs can be reverted.
raise TemplateSyntaxError, | ||
line: 0, | ||
message: "I found a HEEx component, but this is not compiling to a HEEx file", | ||
line_number: 0, | ||
column: 0 | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be useful for debugging to print the HEExNode here or tell the user to look for tags that begin with :
, I think.
alias Slime.TemplateSyntaxError | ||
|
||
@eex_delimiters {"#" <> "{", "}"} | ||
@heex_delimiters {"{", "}"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, the values of these globals are never used, except for comparing if the passed "delimiters" parameter matches one or the other.
Given this, perhaps it would be better to pass a symbol instead? Something like complile(x, :eex)
or compile(x, %{output_format: :eex})
?
end | ||
|
||
@doc """ | ||
Compile Slime template to valid EEx HTML. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compile Slime template to valid EEx HTML. | |
Compile Slime template to valid HEEx HTML. |
iex> Slime.Renderer.precompile(~s(input.required type="hidden")) | ||
"<input class=\\"required\\" type=\\"hidden\\">" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iex> Slime.Renderer.precompile(~s(input.required type="hidden")) | |
"<input class=\\"required\\" type=\\"hidden\\">" | |
iex> Slime.Renderer.precompile_heex(~s(:component.required type="hidden")) | |
"<.component class=\\"required\\" type=\\"hidden\\">" |
""" | ||
def render(slime, bindings \\ [], opts \\ []) do | ||
slime | ||
|> precompile | ||
|> precompile() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|> precompile() | |
|> precompile |
Removing a formatting change to simplify the diff.
``` | ||
|
||
```html | ||
<tt>Always bring a towel.<tt> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be either
"<tt><Always>bring a towel.</Always></tt>"
or
tt
| Always bring a towel.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right. I'll add a suggestion for that, too.
iex(1)> Slime.Renderer.precompile("tt\n Always bring a towel")
"<tt><Always>bring a towel</Always></tt>"
iex(2)> Slime.Renderer.precompile("tt\n | Always bring a towel")
"<tt>Always bring a towel</tt>"
```slim | ||
tt | ||
Always bring a towel. | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
```slim | |
tt | |
Always bring a towel. | |
``` | |
```slim | |
tt Always bring a towel. | |
``` |
If you split the tag across multiple lines you have to use |
.
@cmcaine @tensiondriven apologizes for being absent, some unfortunate events took me away from open source for awhile but I'm getting back into the swing of things. I'd love to move forward with this PR, how can I help? |
This PR adds support to the
slime
package for HEEx. The main changes to the slim/slime grammar are:When rendering HEEx, attribute values will be escapes with
{ }
instead of#{ }
.When rendering HEEx, tag names prefixed with a colon (
:
) will be rendered with a dot (.
).The dot and curlybrace syntax changes are specific to HEEx.
This is accomplished by adding a
precompile_heex
function, similar to the existingprecompile
function. It works by passing in a set of delimiters for escaping attribute values:These delimiters are passed down into
Slime.Compiler
to allow it to return the proper data structures, depending on if HTML or HEEx is requested. Additionally,Slime.Parser.Nodes.HEExNode
was added which is returned inSlime.Parser.Transform
.The PEG grammar was updated to add the case where a tag name is prefixed with a colon (
:
). When precompile (which targets EEx, not HEEx) is called with input that contains a colon, aSlime.TemplateSyntaxError
is raised.Tests were added in
test/rendering/heex_test.exs
. Testing the HEEx functionality end-to-end was difficult because it would add a dependency onphoenix_slime
, so those tests were omitted. This was picked up in my fork ofphoenix_slime
here: https://github.com/tensiondriven/phoenix_slime/blob/master/test/phoenix_slime_heex_test.exsI also set up a test Phoenix app which pulls in the new versions of Slime and PhoenixSlime here:
https://github.com/tensiondriven/phoenix_slime_test
I expect there are still some issues hiding in here somewhere, so please don't merge until enough review has been done. For now, I'm submitting the PR to get the review process going. The test app (linked above) is working and contains as many cases as I could think to test, but I'm sure there are more that are lurking.
Co-Authored-By: Topher Hunt [email protected]