phoenixframework / phoenix_live_view

Rich, real-time user experiences with server-rendered HTML
https://hex.pm/packages/phoenix_live_view
MIT License
6.02k stars 907 forks source link

Proposal: Live View Native upstream changes #2427

Closed bcardarella closed 1 year ago

bcardarella commented 1 year ago

Unfortunately we'll have to violate one of my stated goals of not requiring any upstream changed in LiveView itself. However, at the moment where we've hit roadblocks seem all related to LiveView's HTML Engine and its limits/validations imposed on the templates.

Being that we are producing markup and not actual valid HTML it was inevitable that we'd hit this. For the time being we've unblocked our own client development with an experimental engine but after discussing with @chrismccord he is open to giving us escape hatches so we can avoid having to support an engine.

We'd like to leave this issue open for the time being as we continue to surface the blockers on our end. I'll have one of our devs weigh in on what we currently know.

chrismccord commented 1 year ago

Just a note here before the rest of the commentary – our HTML Engine is mostly an XML Engine. We are barely HTML aware and only ensure proper XML structure. There are a couple things we special case like void tags which I believe is the number 1 biggest issue for native. We also special case the class and style attr in that we allow concat'ing lists if passed, but I don't believe this will cause issues.

Once we have a defined set of escape hatches you need we can see if a couple configurations solve everything or not. It's possible we rename our engine to XMLEngine :)

bcardarella commented 1 year ago

I suspect we may have surfaced all of the issues we'll run across in SwiftUI itself. What I don't know yet is if when we start to cover other Apple UI frameworks what will come about.

supernintendo commented 1 year ago

Thanks @chrismccord! I think there are just a few things we would be looking to hook into to support our needs with LiveView Native, specifically within Phoenix.LiveView.HTMLEngine:

  1. The option to disable void tag enforcement to support platforms where tags with identical naming to HTML tags may behave differently. One example of this is the link tag which is self-closing in HTML but may not be in other platforms (for example, SwiftUI).

  2. The ability to define custom callbacks for handle_token/2 (and perhaps other steps within the parser as well) to support client-specific template syntax, i.e. https://github.com/liveviewnative/live_view_native/pull/5 (not to be confused with LiveView slots; we'll be renaming and reworking that functionality)

One additional challenge is that we would need configuration of Phoenix.LiveView.HTMLEngine to be handled in a way that isn't global as one of our design goals is for a single LiveView Native application to serve multiple target platforms (web, iOS and Android, for example). LiveView Native already supports this by dynamically generating a distinct render/1 function for each platform which allows passing a custom :template_engine (see here). Allowing configuration of the HTML engine avoids the need to do this, but looking ahead I think we'll still need the ability to provide platform-specific configuration as opposed to something global like :trim_on_html_eex_engine.

tldr; We want to be able to customize Phoenix.LiveView.HTMLEngine to support our needs for LiveView Native without affecting the design goals of LiveView upstream. This requires tweaking the template engine to make it more open to customization so we can avoid writing our own engine and instead only provide the hooks we need.

@bcardarella @shadowfacts @carson-katri Let me know if there's anything I missed! 🙂

bcardarella commented 1 year ago

We have to be careful as that issue is referring to the element as "slots" but is not the same behavior or value as LiveView Component slots. We're going to rename them and use a different syntax to avoid the confusion with LiveView Component slots.

chrismccord commented 1 year ago

Yeah I don't follow the slot requirement. if you need a way for the caller to build named sections of markup, then it should be defined at the function component level, not the engine/tag.

bcardarella commented 1 year ago

@chrismccord the confusion is because we called them "slots" and it is colliding with LV Component slots. They're not and we're changing that.

As a matter of example, there are some SwiftUI components that have sub-views that can only be tied to that parent view. Menu for example:

Menu {
 ...
} label: {
    Label("Add Bookmark", systemImage: "book")
} primaryAction: {
    addBookmark()
}

Here you see the label: which takes a Label view. We represented this as:

<menu>
  <:label>Add Bookmark", systemImage: "book</:label>
</menu>

so it wouldn't conflict with the <label> component. It's annoying and confusing that SwiftUI has all of these strange things in it. For now we're changing the syntax to:

<menu>
  <menu:label>Add Bookmark", systemImage: "book</menu:label>
</menu>

so it immediately can be seen to be associated with the <menu> view. This will hopefully clear up this confusion.

supernintendo commented 1 year ago

@chrismccord I think the issue there is that it would require us to maintain a library of function components for every element in SwiftUI (or whatever the target platform is) which we're really trying to avoid. If we could just hook into the syntax parsing of Phoenix.LiveView.HTMLEngine it would allow us to support the level of integration we need between the backend LiveView application and native client running on the device.

bcardarella commented 1 year ago

@supernintendo we wouldn't be breaking actual LV slots, right? Everything that can be done in LV should still be OK to use in LVN

supernintendo commented 1 year ago

@bcardarella Yes, that's true. I think if we make the changes according to your example above that should eliminate the requirement for custom parsing (since <menu:label></menu:label> is valid XML that doesn't conflict with the slots syntax). So for now @chrismccord, I think allowing void / self-closing tags to be optional is the only real blocker.

carson-katri commented 1 year ago

The <menu:label></menu:label> syntax already works fine, since our engine extensions just added sugar to convert <:label> to that.

josevalim commented 1 year ago

Hi everyone! I am completely out of context here, so apologies for the silly question or if I get something wrong.

What would be wrong with:

<menu>
  <label systemImage="book">Add Bookmark</label>
</menu>

This is similar to HTML <tr> and <td>, where you can only have a td inside a tr and anything else is an error.

In fact, I am not a fan of <menu><:label>... because the only thing that can have slots in LiveView is user components, but a regular HTML tag is not a Phoenix component.

So in my mind, all of the building blocks provided by LiveView Native itself are rather equivalent to HTML. And then user components and whatever else do not change and rather build on top of said blocks.

WDYT? Should we have a call to align this all?

bcardarella commented 1 year ago

@josevalim the problem we're dealing with here is that there is both a Label SwiftUI view that we are already representing as <label> and the Menu view takes a label: option which in turn can take a Label view:

Menu {
    Button("Open in Preview", action: openInPreview)
    Button("Save as PDF", action: saveAsPDF)
} label: {
    Label("PDF", systemImage: "doc.fill")
}

To further complicate matters Label can be used arbitrarily within a Menu like any other view., not just within the label::

Menu {
    Button(action: addCurrentTabToReadingList) {
        Label("Add to Reading List", systemImage: "eyeglasses")
    }
    Button(action: bookmarkAll) {
        Label("Add Bookmarks for All Tabs", systemImage: "book")
    }
    Button(action: show) {
        Label("Show All Bookmarks", systemImage: "books.vertical")
    }
} label: {
    Label("Add Bookmark", systemImage: "book")
}

If we just used <label> here there would be disambiguity on the client side as to what was meant to be a view and what was meant to be an option on Menu:

<menu>
   <button><label system-image="eyeglasses">Add to Reading List</label></button>
   <button><label system-image="book">Add Bookmarks for All Tab</label></button>
   <button><label system-image="books.vertical">Show All Bookmarks</label><button>
   <label system-image="book">Add Bookmark</label>
</menu>

To further complicate matters, there are a bunch of SwiftUI Views that have this name collision issue. In the Swift struct constructor these are being passed as arguments to Menu but they themselves can take any hierarchy of views. So we've been going back and forth on how best to represent this in markup.

josevalim commented 1 year ago

Thanks @bcardarella! With all of this information, I think <menu:label> is a solid call and, in case the HTMLTokenizer cannot parse such tags today, a PR that adds : to the valid chars is welcome!

bcardarella commented 1 year ago

Yes I think that's what we're aiming for at the moment:

<menu:label><label system-image="book">Add Bookmark</label></menu:label>

I'll let @supernintendo weigh in if the : requires any upstream changes or not.

bcardarella commented 1 year ago

Although... I wonder if this would be do-able:

<menu>
  <label:>
    <label system-image="book">Add Bookmark></label>
  </label:>
</menu>

We avoid the additional syntax and colliding with slots. It has the added benefit of aligning with the original SwiftUI syntax.

@supernintendo @carson-katri @shadowfacts thoughts?

josevalim commented 1 year ago

@bcardarella speaking from experience as someone who created a language with both label: and :label, that will definitely be source of confusion. :D

The <menu:label ... is repetitive but it makes it clear that the label has a distinct meaning within the menu itself. It could be called <menu-label ... or <mlabel just as well.

bcardarella commented 1 year ago

I think the problem is we're trying map an argument option to markup. We have a convention of how to convert SwftUI structs to markup elements and then we are trying to use the same representation for arguments. I agree that it could be confusing but maybe the original sin here is trying to represent this as a markup element.

bcardarella commented 1 year ago

Our situation is also more complicated because we cannot represent element names in their original form. For example:

<Label>...</Label>

If we could do this it would simplify quite a bit. But with LV expecting anything capitalized to be a Module name our hands our tied.

josevalim commented 1 year ago

FWIW, we could support that with no problems.

bcardarella commented 1 year ago

@josevalim after chatting with @chrismccord he said it wouldn't be possible, is there something I'm missing? Getting access to the proper capitalization for elements would be a huge help IMO

bcardarella commented 1 year ago

Especially when we start to really get into Jetpack development and WinUI3. Those are essentially straight up XML docs

josevalim commented 1 year ago

@bcardarella we should schedule a chat but the limitation today is artificial. At the same time, we don't want people to write <Header> or <TABLE>. But it is something we can enable as a customization. You may need a .neex template though (for native).

bcardarella commented 1 year ago

@josevalim yes I agree, we're finding too many divergent needs that I don't want to impose what we want on top of the HTML needs and constraints of LiveView. Either having escape hatches so we can turn off certain validations/constraints or a different template type would be ideal. But to @chrismccord's original point if we were having to maintain our own Engine it would probably create another world of pain.

I'll DM you to see if we can get onto a call in the next week or two. Thanks!

bcardarella commented 1 year ago

Just to be clear, if we can get the capitalization benefit we can constrain on that for views then options such as this case could be all lower case:

<Menu>
   <Button><Label system-image="eyeglasses">Add to Reading List</Label></Button>
   <Button><Label system-image="book">Add Bookmarks for All Tab</Label></Button>
   <Button><Label system-image="books.vertical">Show All Bookmarks</Label><Button>
   <label><Label system-image="book">Add Bookmark</Label></label>
</Menu>

As far as readability goes, if this were its own .neex template then we could have special syntax highlighting rules to differentiate between the two element types. As it stands if they're both syntax highlighted the same way the readability is poor.

carson-katri commented 1 year ago

@bcardarella Looking into this capitalization solution more today, and I found a few things:

  1. The syntax highlighting (from the Phoenix Framework extension I believe?) does differentiate them, because it treats the capitalized names as module names.
  2. The Rust core library lowercases all tag names (AFAICT, this happens in html5gum which it depends on: https://github.com/untitaker/html5gum/blob/b6fb6c5d3a100217af8bfb8e1da1d200d2feba65/src/machine.rs#LL274C23-L274C23). This will need to be configurable if we want to go this direction.

I also explored a few other possible ideas for this problem, but I posted those on the liveview-client-swiftui repo as it deviates slightly from the discussion of upstream changes: https://github.com/liveviewnative/liveview-client-swiftui/issues/142#issuecomment-1421284360

bcardarella commented 1 year ago

@carson-katri thanks for checking in to this.

  1. for the highlighting, if we do end up having a .neex template or another extension name we could alias to the .heex template highlighting in vscode or wherever they're being used. But if we move over to .neex today we lose the highlighting
  2. I'll chat with Paul about the Rust side of things. He's scheduled to rejoin the LVN project in a week or two to tackle some stuff in Core and I can include this in the scope
chrismccord commented 1 year ago

Cap tags are certainly possible and easy with the engine, but my main concerns were conflating the remote function component call <Label.bar> with an XML tag, <Label>, so I preferred to steer you towards downcast until we otherwise had reason to introduce the mixed usage.

josevalim commented 1 year ago

The only downside of <Label> is the slight ambiguity with remote function components, as in <Label.foo> but it should be manageable specially with syntax highlighting.

bcardarella commented 1 year ago

@josevalim we can probably have the syntax highlighter differentiate when it sees a function component. It seems like a trivial pattern to match

bcardarella commented 1 year ago

@carson-katri I spoke with Paul, he is going to adapt Core to support element casing

josevalim commented 1 year ago

@bcardarella fwiw, if LiveView will send data as <Menu> and you will validate that's the case, then the engine can totally be case insensitive because you have guaranteed that upstream is not case sensitive.

mgibowski commented 1 year ago

I don't think it would be good to have two tags <label> and <Label> with different meanings.

How about going the more explicit way?

<menu>
   <button><label-view system-image="eyeglasses">Add to Reading List</label-view></button>
   <button><label-view system-image="book">Add Bookmarks for All Tab</label-view></button>
   <button><label-view system-image="books.vertical">Show All Bookmarks</label-view><button>
   <label-option system-image="book">Add Bookmark</label-option>
</menu>