-
Notifications
You must be signed in to change notification settings - Fork 149
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
Implement a compiler instead of a translator #15
Comments
How can I view what AST structure LiveView expects? |
Hi @mazz-seven! You can view the translated code of any component or HTML tag by adding the Example: <Hello name="John Doe" :debug/> will print to the standard output something like: <% require Hello %>
<% props = put_default_props(%{ name: "John Doe"}, Hello) %>
<% props = Map.put(props, :__surface__, %{groups: %{__default__: %{binding: false, size: 0}}}) %>
<% props = Map.put(props, :context, context) %>
<% children_props = %{} %>
<%= live_component(@socket, Hello, Keyword.new(Map.merge(props, children_props))) %> |
First, it's very cool to see how things are built behind the scenes. What should be the AST for |
Is this something you're interested in getting help with? |
Hey @lnr0626! This one is definitely going to take a lot of effort so YES, I'd love to get some help :) Since it has a huge impact on almost everything we currently have, my plan is to create a separate project to implement the engine, something like I'll create the project with more information and some initial implementation sometime next week. Then we move on from that. WDYT? Thank you for your help! |
Sounds great! |
@lnr0626 I finally started working on this. What we need to do is to create something similar to the EEX Compiler. The problem is that since EEX doesn't care about the structure of the template (there's no hierarchy), the I believe the only way to make this work is to first convert our tree into a similar list. You can take a look at the shape of the tokens generated by I'm still trying to find out the best way we can cover all Surface features, including directives and slots, using this approach. It will probably take a few days until I have something running so in case you also want to make some experiments or try different approaches, please let me know. |
@msaraiva I have a few ideas I'm going to experiment with, if any of them look promising I'll let you know |
I might have something promising. The idea I had was to take the parse tree we get from the parser, and convert it to a heirarchial AST. The AST has all of the relevant information supporting Surface features (e.g. directives, slots), and should be convertible into a I'll create a draft PR so you can see the code that does this (though it's still super rough around the edges). So far I've been able to implement everything up to the conversion into One practical change for this is that macro components would need to return an AST struct instead of iodata, and the Raw component doesn't support EEx expressions as currently implemented - instead it just outputs static text (e.g. how it's used in the form component to produce the closing I replaced the translator_test with a compiler_test that asserts on the parsed AST instead of the generated EEx code, but here are a few examples of what the AST looks like with a few different inputs: Input: <div></div> AST: [
%Surface.AST.Tag{
attributes: [],
children: [],
directives: [],
element: "div",
meta: #Surface.AST.Meta<
file: "nofile",
line: 1,
line_offset: 0,
module: nil,
node_alias: nil,
...
>
}
] Input: <LivePatch to={{ @href }} class={{ "some class", potential: @test }}>
A link to <strong>something awesome</strong>.
</LivePatch> Output: [
%Surface.AST.Component{
directives: [],
meta: #Surface.AST.Meta<
file: "nofile",
line: 1,
line_offset: 0,
module: Surface.Components.LivePatch,
node_alias: "Surface.Components.LivePatch",
...
>,
module: Surface.Components.LivePatch,
props: [
%Surface.AST.Attribute{
meta: #Surface.AST.Meta<
file: "nofile",
line: 1,
line_offset: 0,
module: Surface.Components.LivePatch,
node_alias: "Surface.Components.LivePatch",
...
>,
name: :to,
type: :string,
value: [
%Surface.AST.AttributeExpr{
meta: #Surface.AST.Meta<
file: "nofile",
line: 1,
line_offset: 0,
module: Surface.Components.LivePatch,
node_alias: "Surface.Components.LivePatch",
...
>,
original: " @href ",
value: {:@, [line: 1], [{:href, [line: 1], nil}]}
}
]
},
%Surface.AST.Attribute{
meta: #Surface.AST.Meta<
file: "nofile",
line: 1,
line_offset: 0,
module: Surface.Components.LivePatch,
node_alias: "Surface.Components.LivePatch",
...
>,
name: :class,
type: :css_class,
value: [
%Surface.AST.AttributeExpr{
meta: #Surface.AST.Meta<
file: "nofile",
line: 1,
line_offset: 0,
module: Surface.Components.LivePatch,
node_alias: "Surface.Components.LivePatch",
...
>,
original: " \"some class\", potential: @test ",
value: {{:., [line: 1],
[{:__aliases__, [line: 1], [:Surface]}, :css_class]}, [line: 1],
[
"some class",
[potential: {:@, [line: 1], [{:test, [line: 1], nil}]}]
]}
}
]
}
],
templates: %{
default: [
%Surface.AST.Template{
children: [
%Surface.AST.Text{value: "\n A link to "},
%Surface.AST.Tag{
attributes: [],
children: [%Surface.AST.Text{value: "something awesome"}],
directives: [],
element: "strong",
meta: #Surface.AST.Meta<
file: "nofile",
line: 2,
line_offset: 0,
module: Surface.Components.LivePatch,
node_alias: "Surface.Components.LivePatch",
...
>
},
%Surface.AST.Text{value: ".\n"}
],
meta: #Surface.AST.Meta<
file: "nofile",
line: 1,
line_offset: 0,
module: Surface.Components.LivePatch,
node_alias: "Surface.Components.LivePatch",
...
>,
name: :default,
props: :no_props
}
]
}
},
%Surface.AST.Text{value: "\n"}
] Input:
Output:
With an error:
And then converting that to the sequence of static and dynamic pieces:
I'm happy to keep moving forward on this, but wanted to run it by you before going too much further. WDYT? |
Hey @lnr0626! This is absolutely fantastic! It looks very promising indeed. I can see already that it would make it much easier for us to maintain and extend Surface using this approach. One thing that is not clear to me yet is how do you intend to integrate with Phoenix.LiveView.Engine. The I think if we can solve this last step, we can certainly move forward with this approach. Thoughts? |
@lnr0626 you're right. As far as I know, there's currently no way to correctly implement the offset for both, |
I'll see what I can come up with for integrating with the LiveView EEx engine - I have the start of an idea, but I need to spend some time looking at the EEx tokenizer and compiler to flesh it out. |
@msaraiva surface interpolation and attribute expressions require full expressions, corrrect? i.e. there's no equivalent to
where the |
@lnr0626 correct. We don't accept that in Surface and probably never will :) |
@lnr0626 I took another look at the code and I'm really excited about this. Please let me know whenever you feel it's time for me to jump in. I'll put everything else on hold until we have this done. If you prefer, I can create a new branch on the main project and give you write access so we could continue the work there. If you need to contact me for questions or any assistance, feel free to DM me on the Elixir Forum or ping me on the Thank you so much for working on this! |
@msaraiva It's exciting to see it starting to work (something like 3/4 of the tests are passing now!). I don't have a strong preference, switching to a branch on the main project works for me. I'm going to spend some time today or tomorrow organizing the current state (i.e. what's working, what isn't working), and looking through the TODOs I've left sprinkled throughout the code - that'll give us a better idea of what's left before this is ready |
Wow! 3/4 sounds really amazing :) |
Alright, so the current state of this: of the 399 tests, 383 are passing and 16 are failing (96%!!!). I think there's just one remaining issue that once fixed will address the 16 remaining tests. All of these tests are failing with the same error which indicates that I'm not rendering components within components correctly. For reference, it's this error:
I'm also seeing a bunch of warnings about the A couple questions to consider:
If you have any thoughts/suggestions for making the code clearer/easier to read/generally better I'm all for that. I'd also appreciate a second set of eyes as a sanity check with all of the quoting/unquoting - some of that code in |
Fantastic! 🎉
Looking into it now.
It might be useful but since we don't need to make that distinction now, I'd should just leave it as it is.
I don't think so, for now.
I'm not sure it's required. I'll think of more examples of its use.
Makes sense.
Yeah, I'm still getting familiar with the logic. As soon as we fix the tests, I'll take a close look at the whole solution. Regarding |
That fixed it, thanks!
That's a good idea - and would be far more useful that what I currently have for
I figured out a different way to handle the filtering for nil sessions (34a2b36) - I was originally handling |
Resolved in #98. |
Generate the AST directly instead of an EEx template
The text was updated successfully, but these errors were encountered: