Implement a compiler instead of a translator about surface HOT 22 CLOSED

We need to check if do_block is []. If so, we have to generate just:

That fixed it, thanks!

Regarding :debug, I think we should change it so that instead of a boolean we have something else that we can specify at what step (or steps) we want to debug. Currently, it prints the Surface.AST but right now, for instance, I'm mostly interested in the final elixir code generated which I'm manually printing inside to_expression/3.

That's a good idea - and would be far more useful that what I currently have for :debug

I'm not sure it's required. I'll think of more examples of its use.

I figured out a different way to handle the filtering for nil sessions (34a2b36) - I was originally handling Surface.LiveView the same way as Surface.(Live)?Component, but with pulling them apart it makes sense to just filter out nil props for live views (nil for either session or id is an error for live view, though not including session appears to work just fine).

from surface.

Hi @mazz-seven!

You can view the translated code of any component or HTML tag by adding the :debug directive.

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))) %>

from surface.

@msaraiva I have a few ideas I'm going to experiment with, if any of them look promising I'll let you know

from surface.

@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

from surface.

I'm also seeing a bunch of warnings about the assigns variable being unused

@lnr0626 the warnings are related to the live_component call being generated as:

like_component(@socket, ...) do
  []
end

We need to check if do_block is []. If so, we have to generate just:

like_component(@socket, ...)

from surface.

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 surface_engine. As soon as the engine is able to translate all features we have, including directives and slots, we can then replace the translator with the new compiler.

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!

from surface.

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 Phoenix.LiveView.Rendered struct. We can perform the same validation on the AST as we currently perfom in the various translators, and implement directives as transformations on the AST. Once we've created the AST and done all of the transformations on it, we can convert it into a list containing static strings, dynamic ast nodes, and nested lists of the same (I'll add some examples to better show what I'm talking about). We can then flatten that list, combine all adjacent static portions, then convert that list into a Phoenix.LiveView.Rendered struct.

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 Phoenix.LiveView.Rendered. All of the tests around syntax and compile time validation pass, but none of the tests that assert on the final rendered content pass (I haven't worked on creating that Rendered struct yet). I also have it creating that sequence of static and dynamic portions (I wanted to see how difficult it was to get to that point before calling this promising). There are also some issues around line numbers (I think it has to do with the difference between ~H() and ~H""" """ - __CALLER__.line for both heredocs and the other are the same, event though heredocs have a single line offset before the start of the code).

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 </form> tag).

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:

<Surface.Components.LivePatch to={{ @to }} />

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: " @to ",
            value: {:@, [line: 1], [{:to, [line: 1], nil}]}
          }
        ]
      }
    ],
    templates: %{default: []}
  }
]

With an error:

iex> "<NonExistant />" |> Surface.Compiler.compile(0, __ENV__)
warning: cannot render <NonExistant> (module NonExistant could not be loaded)
  iex:1: (file)

[
  %Surface.AST.Error{
    message: "cannot render <NonExistant> (module NonExistant could not be loaded)",
    meta: #Surface.AST.Meta<
      file: "nofile",
      line: 1,
      line_offset: 0,
      module: nil,
      node_alias: nil,
      ...
    >
  }
]

And then converting that to the sequence of static and dynamic pieces:

iex> ~S(<div><span class={{ @class }}><Surface.Components.LivePatch to="/path" /></span></div><div>{{ @some_content}}</div>) |> Surface.Compiler.compile(0, __ENV__) |> Surface.Compiler.Renderer.render()
[
  "<div><span class=\"",
  %Surface.AST.AttributeExpr{
    meta: #Surface.AST.Meta<
      file: "nofile",
      line: 1,
      line_offset: 0,
      module: nil,
      node_alias: nil,
      ...
    >,
    original: " @class ",
    value: {{:., [line: 1], [{:__aliases__, [line: 1], [:Surface]}, :css_class]},
     [line: 1], [{:@, [line: 1], [{:class, [line: 1], nil}]}]}
  },
  "\">",
  %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.Text{value: "/path"}]
      }
    ],
    templates: %{default: []}
  },
  "</span></div><div>",
  %Surface.AST.Interpolation{
    meta: #Surface.AST.Meta<
      file: "nofile",
      line: 1,
      line_offset: 0,
      module: nil,
      node_alias: nil,
      ...
    >,
    value: {:@, [line: 1], [{:some_content, [line: 1], nil}]}
  },
  "</div>"
]

I'm happy to keep moving forward on this, but wanted to run it by you before going too much further. WDYT?

from surface.

There are also some issues around line numbers (I think it has to do with the difference between ~H() and ~H""" """ - __CALLER__.line for both heredocs and the other are the same, event though heredocs have a single line offset before the start of the code).

@lnr0626 you're right. As far as I know, there's currently no way to correctly implement the offset for both, ~H() and ~H""", so we have to pick one (just like Phoenix does). Since ~H""" is the most used, I think we should keep the __CALLER__.line + 1 and don't use any of the other ~H* variants whenever the line info matters, e.g. when testing errors/warnings.

from surface.

@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 #surface channel at https://elixir-lang.slack.com.

Thank you so much for working on this!

from surface.

How can I view what AST structure LiveView expects?

from surface.

First, it's very cool to see how things are built behind the scenes.

What should be the AST for <Hello name="John Doe" :debug/>? I thought there is a special AST that LiveView expects. And that you want to go from <Hello name="John Doe" :debug/> to AST.

from surface.

Is this something you're interested in getting help with?

from surface.

Sounds great!

from surface.

@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 EEx.Tokenizer.tokenize/3 can easily generate a list of sequential tokens, but Surface, on the other hand, needs to know the structure of the template so our parser must generate a tree of nodes instead.

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 EEx.Tokenizer.tokenize/3 here. It doesn't have to be exactly the same structure as long as we can read it sequentially.

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.

from surface.

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 Phoenix.LiveView.Rendered struct is supposed to be private so we shouldn't create it by hand. The "right" way to implement what we want would be by either emitting an EEx template or calling the EEx engine hooks directly. Otherwise, we'll need to reimplement lots of internal things like change tracking.

I think if we can solve this last step, we can certainly move forward with this approach.

Thoughts?

from surface.

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.

from surface.

@msaraiva surface interpolation and attribute expressions require full expressions, corrrect? i.e. there's no equivalent to

<%= if @condition do %>
<!-- some html -->
<% end %>

where the if ... do and end show up in separate interpolation blocks.

from surface.

@lnr0626 correct. We don't accept that in Surface and probably never will :)

from surface.

It's exciting to see it starting to work (something like 3/4 of the tests are passing now!)

Wow! 3/4 sounds really amazing :)

from surface.

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:

 12) test context assingns are scoped by their parent components (ContextTest)
     test/context_test.exs:111
     ** (ArgumentError) cannot convert component ContextTest.InnerWrapper with id nil to HTML.
     
     A component must always be returned directly as part of a LiveView template.
     
     For example, this is not allowed:
     
         <%= content_tag :div do %>
           <%= live_component @socket, SomeComponent %>
         <% end %>
     
     That's because the component is inside `content_tag`. However, this works:
     
         <div>
           <%= live_component @socket, SomeComponent %>
         </div>
     
     Components are also allowed inside Elixir's special forms, such as
     `if`, `for`, `case`, and friends. So while this does not work:
     
         <%= Enum.map(items, fn item -> %>
           <%= live_component @socket, SomeComponent, id: item %>
         <% end %>
     
     Since the component was given to `Enum.map/2`, this does:
     
         <%= for item <- items do %>
           <%= live_component @socket, SomeComponent, id: item %>
         <% end %>
     
     stacktrace:
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/engine.ex:20: Phoenix.HTML.Safe.Phoenix.LiveView.Component.to_iodata/1
       anonymous fn/3 in ContextTest.TestLiveView_13188.render/1
       (surface 0.1.0-alpha.2) lib/surface/content_handler.ex:72: anonymous fn/4 in Surface.ContentHandler.join_contents/3
       (surface 0.1.0-alpha.2) lib/surface/content_handler.ex:72: anonymous fn/4 in Surface.ContentHandler.join_contents/3
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/diff.ex:307: Phoenix.LiveView.Diff.traverse/6
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/diff.ex:381: anonymous fn/4 in Phoenix.LiveView.Diff.traverse_dynamic/6
       (elixir 1.10.4) lib/enum.ex:2111: Enum."-reduce/3-lists^foldl/2-0-"/3
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/diff.ex:307: Phoenix.LiveView.Diff.traverse/6
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/diff.ex:381: anonymous fn/4 in Phoenix.LiveView.Diff.traverse_dynamic/6
       (elixir 1.10.4) lib/enum.ex:2111: Enum."-reduce/3-lists^foldl/2-0-"/3
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/diff.ex:307: Phoenix.LiveView.Diff.traverse/6
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/diff.ex:381: anonymous fn/4 in Phoenix.LiveView.Diff.traverse_dynamic/6
       (elixir 1.10.4) lib/enum.ex:2111: Enum."-reduce/3-lists^foldl/2-0-"/3
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/diff.ex:307: Phoenix.LiveView.Diff.traverse/6
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/diff.ex:115: Phoenix.LiveView.Diff.render/3
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/static.ex:284: Phoenix.LiveView.Static.to_rendered_content_tag/4
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/static.ex:144: Phoenix.LiveView.Static.render/3
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/controller.ex:35: Phoenix.LiveView.Controller.live_render/3
       (phoenix_live_view 0.14.4) lib/phoenix_live_view/test/live_view_test.ex:224: Phoenix.LiveViewTest.__isolated__/4
       test/test_helper.exs:77: ComponentTestHelper.render_live/3

I'm also seeing a bunch of warnings about the assigns variable being unused; however, my intuition is that that's due to the same issue as is causing the components to not render correctly. If fixing the first issue doesn't address this warning, then that may take some more sleuthing to figure out the root cause. Once those tests are passing, the main thing left is to clean up the unused code paths.

A couple questions to consider:

• do we want to be able to distinguish between plan text and raw html in the AST? currently I have Surface.AST.Text which is used for static attribute values and static html content. Current I am html-escaping attribute values, but not AST.Text nodes that are interpreted as html content - see https://github.com/msaraiva/surface/blob/cabca964a687fbd28479cce68fc910be92c0e40e/lib/surface/compiler/eex_engine.ex#L591-L599 vs https://github.com/msaraiva/surface/blob/cabca964a687fbd28479cce68fc910be92c0e40e/lib/surface/compiler/eex_engine.ex#L65
• is there any additional validation we want to do for live views/live components? currently it's limited to the same single root element check for live components
• I added an opt to props called reject_nil that's used to prevent attempting to filter out an unset session property or one set to nil. is that the behavior we want? or is that property actually required?
• the macro component and component handling is now split apart, with macro components only getting a subset of directives, parsing out the properties into the AST, then expanding the macro. The directives that are allowed are :for, :if, and :debug (more just a note than anything else)

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 Surface.Compiler.EExEngine is fairly complicated.

from surface.

so the current state of this: of the 399 tests, 383 are passing and 16 are failing (96%!!!).

Fantastic! 🎉

I think there's just one remaining issue that once fixed will address the 16 remaining tests.

Looking into it now.

do we want to be able to distinguish between plan text and raw html in the AST?

It might be useful but since we don't need to make that distinction now, I'd should just leave it as it is.

is there any additional validation we want to do for live views/live components?

I don't think so, for now.

I added an opt to props called reject_nil that's used to prevent attempting to filter out an unset session property or one set to nil. is that the behavior we want? or is that property actually required?

I'm not sure it's required. I'll think of more examples of its use.

the macro component and component handling is now split apart, with macro components only getting a subset of directives, parsing out the properties into the AST, then expanding the macro. The directives that are allowed are :for, :if, and :debug (more just a note than anything else)

Makes sense.

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 Surface.Compiler.EExEngine is fairly complicated.

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 :debug, I think we should change it so that instead of a boolean we have something else that we can specify at what step (or steps) we want to debug. Currently, it prints the Surface.AST but right now, for instance, I'm mostly interested in the final elixir code generated which I'm manually printing inside to_expression/3.

from surface.

Resolved in #98.

from surface.