liveview-native / liveview-client-swiftui

MIT License
353 stars 31 forks source link

Proposal on modifiers #1132

Closed bcardarella closed 7 months ago

bcardarella commented 10 months ago

Using the following class name syntax:

<function1>(<value1> <value2>){<contentName>} <functionN>(<valueN>){<contentName>}

we can call any modifier that we map to match.

<Text :modifiers="font(.largeTitle) bold(true) italic(true)">Hello!</Text>

This would be the same in SwiftUI as:

Text("Hello")
  .font(.largeTitle)
  .bold(true)
  .italic(true)

While this isn't quite Swift code it is very close. We'd need to parse and map to the correct Swift function so we're not dynamically executing code or breaking the remote code execution policy. Only the functions we include in the map are called. If something doesn't exist we should warn to the logger and ignore the entire function call.

The parser on this could be written to be quite fast and an order of N. If the string is parsed from left to write and an AST is built

"font(.largeTitle) bold italic"

would get parsed as [["font", [.largeTitle], nil],["bold", [true], nil],["italic", [true], nil]]

the tuple represents

[<function>, <argsList>, <contentName>]

argList must always be an array of values.

If when looking up the function and the number of arguments supplied doesn't match what is accepted the className should ignored but a warning thrown explaining why.

class name format

<className1> <className2>

className must be a <function> from the Argument Value Forms below.

Argument value forms

  1. number - if the value is a valid number . We should support negative numbers -10 and floats 0.5. This is represented in the tuple as a single value 123
  2. implicit member expression - if the value starts with a . for example .red now becomes the Swift implicit member expression .red. Chaining should be permitted as well.
  3. bool - if the value is "true" or "false" these should convert to true and false. This is represented in the tuple as a single value: true
  4. nil - if the value is "nil" convert to nil this is represented in the tuple as a single value nil
  5. function call - if a <function> is immediately followed by an open hyphen then it will parse until the closing hyphen for white space separated arugments. If the value contains a function call it should ensure white space within the function call isn't parsed as the next argument this is represented in the tuple as its own tuple [<functionName>, <argList>, <contentName>] <functionName> must have a string value. <argList> must be a list of zero - N values in an array [ ... ] and each value is itself in an argument value tuple form. <contentName> is either nil or a String.
  6. key/value - if <key>:<value> is matched parse value as its argument value form. No whitespace is permitted between the <key> and the : not between the : and the <value>. This is represented in the tuple as [<key>, <value>]. <key> must be a String. <value> must follow the argument value form tuple.
  7. text - if all other forms fail to match assume text. This is represented as a single "..." value in the tuple

whitespace characters indicate next argument value.

<function> must always follow the casing of the actual modifers. For example, for allowsTightening:

<Text class="alowsTightening(true) font(.largeTitle) bold italic>Hello!</Text>

For this swiftUI example:

Text("ABCDEF")
    .background(alignment: .leading) { Star(color: .red) }
    .background(alignment: .center) { Star(color: .green) }
    .background(alignment: .trailing) { Star(color: .blue) }

we could represent this as

<Text class="background(alignment:.leading){StarRed} background(alignment:.leading){StarGreen} background(alignment:.leading){StarBlue}>
ABCDEF
    <StarRed>
        <Star class="color-red"/>
    </StarRed>
    <StarGreen>
        <Star class="color-green"/>
    </StarGreen>
    <StarBlue>
        <Star class="color-blue"/>
    <StarBlue>
</Text>

We'd probably want a way to throw an error if <contentName> matched an existing SwiftUI view name. This would lead to an error where if this were done:

<VStack class="background{Text}">
   <Text>
      <Text>This will render</Text>
   </Text>
   <Text>This won't</Text>
</VStack>

int this example because contentName was set to Text is correctly removed the first matched Text element but also removed the second Text element from the tree. If a different name was chosen such as:

<VStack class="background{MoreText}">
   <MoreText>
      <Text>This will render</Text>
   </MoreText>
   <Text>This will also now render</Text>
</VStack>

But because int he client we already have a map of all LVN views through our Registry we can just look up if <contentName> is already defined as a LVN view then error if it is.

This is a very verbose format but I believe it will meet our needs to get the diff/patch benefits of LiveView for the modifiers which currently it doesn't. It also avoids the serialization overhead. It introducers a verbose but logical syntax that anybody can look up a modifiers and know how to call it with class names.

This should allow for faster modifier parsing. It should also allow for cleaning diff pathing in LiveVie itself. This could lead to

bcardarella commented 10 months ago

@josevalim if we use the slot syntax what happens when someone wants to encapsulate LVN behavior and use slots within a component? Are we setting ourselves up for failure by making use of that syntax here?

bcardarella commented 10 months ago

@carson-katri so I agree with @supernintendo that option 2 encapsulates "searchable" best but what about for modifiers like:

Text("Hello")
  .font(.largeTitle)
  .bold(true)
  .italic(true)

It would seem incredibly verbose to do similar syntax for these types of modifiers

carson-katri commented 10 months ago

Since they don’t have multiple arguments, they can be represented with just a class name.

font-largetitle bold italic

supernintendo commented 10 months ago

@bcardarella Wouldn't it just be up to the app developer to define which class names apply certain modifiers? If I wanted to use "large-title" as a class name and map that to .font(.largeTitle) from the custom registry in my app I should be able to do that. I think the custom syntax should only go so far as to provide workarounds for any limitations, but aside from that we should be un-opinionated about what class names developers want to use.

bcardarella commented 10 months ago

Ok so we're still talking about some client side logic to map class names back to modifiers in some form?

josevalim commented 10 months ago

For search in particular you could also make it a slot only for NavigationSplitView and NavigationStack and use the placement to control where it is added, right? You could also just use a custom element, that is only allowed in certain cases. HTML has many tags that only work in certain scenarios.

But I think it goes back to the discussion that you won't be able to support all modifiers with a single elegant syntax, so perhaps it is all about making the component building expressive on Swift side.

Are we setting ourselves up for failure by making use of that syntax here?

I don't think so, they shouldn't conflict with each other as far as I can see.

bcardarella commented 10 months ago

There are two spikes to try out the AST style. We should be able to see quickly if this is having performance overhead:

  1. https://github.com/liveview-native/live_view_native_stylesheet is the base library for compiling stylesheets
  2. @TwistingTwists has helped by spiking most of the rule parsing lib: https://github.com/TwistingTwists/swift_class

This approach will allow us to do runtime substitutions for element attrs into modifier function values, have easily composable and build stylesheets. The rules are compiled on the server and sent along to the client.

While we should support:

<Text :modifiers="color(.red)"></Text>

this approach will still be slow but devs should be able to add styling information directly to the markup if they want. The stylesheet approach essentially will do the same as CSS sheets. We have a collection of rules built up in a map. At runtime the class names on the element are parsed then map/reduced to apply the modifiers parsed from the rule set to the caller.

We can do the following now:

<Text class="color-red"></Text>

and get all of the same benefits of the diff/patch from LiveView

A defined sheet will look like:

defmodule MySwiftUISheet do
  use LiveViewNative.Stylehseet, :swiftui

  def class("color-"<>color_name, _target) do
     "color(.#{color_name})"
  end

  def class("searchable", target: :watchos) do
    "searchable(placeholder: attr(placeholder))"
  end

  def class(unmatched, target: target) do
    {:unmatched, "Could not find #{inspect(unmatched)} for #{inspect(target)}"
  end
end

MySwiftUISheet.compile(["color-red", "searchable"])
bcardarella commented 10 months ago

After speaking with @supernintendo yesterday I've been giving some additional thought to the syntax of the template/content. If we are not currently making use of id in the templates then maybe this is going to be the way to go:

background(alignment: .leading){#starRed}

this would be an id reference to:

<Text class="star-red">
   ABCDEF
   <Star id="starRed" class="color-red" />
</Text>

at runtime the viewtree builder in Swift would be informed as to the template ID and it will find and remove that branch from the DOM tree. That branch would be built as its own view tree in SwiftUI then injected into the modifier:

branch = Star().color(.red)

Text("ABCDEF")
  .background(alignment: .leading) {
    branch
  }

this form can work for any number of modifiers provided the ID within the scope of the element that the modifiers are being applied to is unique and singular:

Text("ABCDEF")
    .background(alignment: .leading) { Star(color: .red) }
    .background(alignment: .center) { Star(color: .green) }
    .background(alignment: .trailing) { Star(color: .blue) }
<Text class="star-red star-green star-blue">
  ABCDEF
  <Star id="starRed" class="color-red" />
  <Star id="starGreen" class="color-green">
  <Star id="starBlue" class="color-blue">
</Text>
def class("star-" <> star_color, _target) do
   "background(alignment: .leading){#star#{String.capitalize(star_color)})"
end

This has a few benefits:

  1. less verbose syntax. We no longer require a wrapping element
  2. doesn't make use of the slot syntax in LiveView which I was never quite comfortable with. Despite geting @josevalim's sign-off it felt like we were creating a problem with confusing people as to what the immediate role of the slot elements are.

The downsides:

  1. if we are already using id this could be a problem
  2. I don't know if the document parser we're using will be strict or not about duplicate IDs in the same document

We'd probably want to also enforce the following:

  1. the syntax within {#starRed} must conform to the ID syntax format, meaning it should always lead with # and throw a compilation error on the Elixir side if it doesn't
  2. on the SwiftUI side if a matching template cannot be found as a child of the view the modifier is being applied to then we should probably ignore that modifier and warn to the logger.
carson-katri commented 10 months ago

@bcardarella We use id, it maps to the Views ID in SwiftUI. Having duplicate IDs can mess with animations and View hierarchy updates.

Otherwise, this looks very similar to what we already have with template="…", just with a shorter name.

supernintendo commented 10 months ago

@bcardarella I just wanted to point out that id maps to the id modifier in SwiftUI so we might need to go with something else unless we're okay with overloading the functionality of that attribute.

bcardarella commented 10 months ago

ah ok, I had a recollection that we were using id. I'm OK with keeping the template= so this would be rewritten as:

<Text class="star-red star-green star-blue">
  ABCDEF
  <Star template="starRed" class="color-red" />
  <Star template="starGreen" class="color-green">
  <Star template="starBlue" class="color-blue">
</Text>

and now we don't need to require # in the name:

def class("star-" <> star_color, _target) do
   "background(alignment: .leading){star#{String.capitalize(star_color)})"
end
carson-katri commented 10 months ago

I still don’t understand why we need to write Swift syntax in Elixir. Can’t we just create an API in the Swift client that lets them write these classes there? It will be faster and require less data to be sent. Plus you’ll get all the code completion, syntax highlighting, type checking, etc for free.

bcardarella commented 10 months ago

@carson-katri the consumers are Elixir devs. I want to reduce the amount of exposure to Swift

carson-katri commented 10 months ago

The proposed syntax looks closer to Swift than Elixir from what I can tell. I think the modifier functions we have now are closer to Elixir. If we package those up into a stylesheet that gets sent to the client once on page load (instead of inlining to modifiers=…), would that provide the same benefits? Then we don’t have to write and manage parsing code at all, since the Elixir compiler does that for us.

def class("star-" <> star_color, _target) do
   background(alignment: :leading, content: "star#{String.capitalize(star_color)}")
end
bcardarella commented 10 months ago

I think supporting the Elixir syntax arguably is a heavier lift for us. This requires us to maintain modifier definitions on both the client and server side. I'm not opposed as this would allow us to drop the parser and have more fine-tuned control over the outputting AST.

bcardarella commented 10 months ago

The proposed syntax looks closer to Swift than Elixir

This is by design as it will allow easy mental-mapping of the actual Swift modifier syntax to the rule based syntax on the server-side

bcardarella commented 10 months ago

Another downside to the Elixir implemented modifiers is we'd need one of two forms, either a list form or a pipe form. The pipe form would require us to carry an accumulator as the first arg between all functions:

native
|> background(...)
|> color(...)

the list form would require us to wrap everything in the definition:

[
  background(...),
  color(...)
]
bcardarella commented 10 months ago

I think too another argument for the rule syntax is we cannot represent chained IMEs in Elixir code:

.red.large

doesn't work with atoms:

:red:large isn't valid Elixir syntax.

carson-katri commented 10 months ago

We already have a working solution for the initial pipe element. If you want to avoid the pipe entirely, maybe a macro like defclass (or something) that allows each line to have a modifier could work?

defclass ("color-" <> color, target) do
  background(...)
  color(...)
end

We already have almost every modifier setup on the Elixir and Swift sides, so it'd just be incremental changes each year from now on.

For the chained values, we currently represent them as a list of atoms in a few cases (such as symbol variants): [:red, :large].

I do think there's benefit to having client-side classes as well. If we create a Tailwind-like library of classes, it will get very large. If the classes were client-side we'd avoid a lot of parsing/encoding/decoding overhead.

bcardarella commented 10 months ago

I do think there's benefit to having client-side classes as well. If we create a Tailwind-like library of classes, it will get very large. If the classes were client-side we'd avoid a lot of parsing/encoding/decoding overhead.

☝️ this. We can pre-compile the stylesheet and re-compile in the DEV environment on code reloads. While the total styling lib may be large the compiled stylesheet will always be a subset and faster.

I don't like the macro form as it gives us elixir-lite and that will be confusing. For now I'd like to roll with the parsed rules as that code is done and we can use it to vet if there is performance overhead on this direction or not. We can easily revisit the syntax topic after/if we confirm that this is the right direction for perf.

bcardarella commented 10 months ago

I gave this some thought and I think @carson-katri 's suggestion to preserve Elixir-like syntax is the best direction. We can address my concern around the piping by having a DSL to define modifiers, which I think @supernintendo's current implementation is doing anyway, and just overload the generated functions by having a style without accumulators and one with:

define_modifier :background, [:int, :keyword_list], content: true

This will produce two functions that have argument constraints:

background(1, foo: "bar"), do: :starRed
background(accumulator, 1, foo: "bar"), do: "starRed"

this way we can start the pipe with a non-accumulator version and then continue to pipe into it. We can use a guard statement to ensure the accumulator style doesn't match against anything else. And we can use a struct for the accumulator to enforce that.

def background(%LiveViewNative.Stylesheet.Accumulator = accumulator, int_1, keyword_list_1) when is_list(accumulator) and is_integer(int_1) and is_list(keyword_list_1) do
end

def background(int_1, keyword_list_1) when is_integer(int_1) and is_list(keyword_list_1) do
end

there are probably more constraints we can impose to ensure that the proper types are passed into the correct arguments for each modifier. This would give us a higher guarantee that any compilation errors can be solved on the Elixir side rather than on the Swift side as runtime errors.

AZholtkevych commented 9 months ago

Related PR is https://github.com/NduatiK/swift_class/pull/2

AZholtkevych commented 7 months ago

According to @carson-katri that has been finished

bcardarella commented 7 months ago

Yes, we went in a different direction ultimately