liveview-native / live_view_native_stylesheet

MIT License
10 stars 4 forks source link

Interpolating nil results in empty string #59

Closed bcardarella closed 5 months ago

bcardarella commented 7 months ago

Because we are using EEx for template compilation if we have a value being interpolated that is nil the result is "" instead of the expected nil being injected into the document.

bcardarella commented 5 months ago

I started to implement this fix but cannot fix without entirely re-writing the EEx compiler, which pretty much defeats the purpose of using EEx.

@NduatiK we should chat to see if we can do the interpolation with the parser instead.

NduatiK commented 5 months ago

I'll need to do a bit more research, but I don't believe the parser will have access to the variables being interpolated using EEx.

The parser and EEx produce code that will at runtime receive a nil; however, since it is only EEx that "understands" variables, changing the parser won't help.


Alternative solution

This line is what converts nil into "". What do you think about writing an EEx engine that delegates all functions to the default SmartEngine, but intercepts the handle_expr function and rewrites nil into "nil"? It might require a bit of metaprogramming to expand the expressions early, but should be doable.

This approach is not new and is in fact what is implemented by the EEx.SmartEngine.

After that, we can provide our engine to EEx.compile_string.

bcardarella commented 5 months ago

There are a few reasons to deviate from EEx. Right now all of the EEx syntax is supported, multiline, comments, etc... which we don't really want.

Instead I think I'll write a byte pre-parser. So it will porcess the ~RULES content byte by byte. If it encounters { start interpolation and I can handle it as I see fit. If it encounters } end interpolation.

NduatiK commented 5 months ago

Aah, I think I understand now, you want to entirely remove EEx. I misread your first comment as "I want to use EEx but don't like how it deals with nil".

NduatiK commented 5 months ago

@bcardarella, if you get the pre-parser (called tokenizer in EEx) working, you can use the following code to compile the code.

I'm assuming you will convert the source content into a list of expressions ({:expr, "1 + b"}) or static text ({:text, "rule-1"})

defmodule LiveViewNative.Stylesheet.RulesParser do
  @moduledoc false

  defmacro sigil_RULES({:<<>>, _meta, [rules]}, _modifier) do
    opts = [
      file: __CALLER__.file,
      line: __CALLER__.line + 1,
      module: __CALLER__.module,
      variable_context: nil
    ]

    compiled_rules =
      rules
      |> tokenize()
      |> compile_string()

    quote do
      LiveViewNative.Stylesheet.RulesParser.parse(unquote(compiled_rules), @format, unquote(opts))
    end
  end

  def tokenize(str) do
    # TODO: Make more robust
    String.split(str, "{")
    |> Enum.flat_map(&String.split(&1, "}"))
    |> Enum.with_index(fn str, i ->
      if rem(i, 2) == 0 do
        {:text, str}
      else
        {:expr, str}
      end
    end)
  end

  def compile_string(tokens) do
    tokens
    |> Enum.reduce(%{vars_count: 0, dynamic: [], binary: []}, &handle_token/2)
    |> then(fn state ->
      %{binary: binary, dynamic: dynamic} = state
      binary = {:<<>>, [], Enum.reverse(binary)}
      dynamic = [binary | dynamic]
      {:__block__, [], Enum.reverse(dynamic)}
    end)
  end

  def handle_token({:text, str}, state) do
    %{state | binary: [str | state.binary]}
  end

  def handle_token({:expr, str}, state) do
    var = Macro.var(:"arg#{state.vars_count}", __MODULE__)
    ast = Code.string_to_quoted!(str, [])

    ast =
      quote do
        unquote(var) = String.Chars.to_string(unquote(ast))
      end

    segment =
      quote do
        unquote(var) :: binary
      end

    %{
      state
      | binary: [segment | state.binary],
        dynamic: [ast | state.dynamic],
        vars_count: state.vars_count + 1
    }
  end

  def fetch(format) do
    with {:ok, plugin} <- LiveViewNative.fetch_plugin(format),
         parser when not is_nil(parser) <- plugin.stylesheet_rules_parser do
      {:ok, parser}
    else
      :error ->
        {:error, "No parser found for `#{inspect(format)}`"}
    end
  end

  def parse(body, format, opts \\ []) do
    case fetch(format) do
      {:ok, parser} ->
        opts =
          opts
          |> Keyword.put_new(:variable_context, Elixir)
          |> Keyword.update(:file, "", &Path.basename/1)

        body
        |> String.replace("\r\n", "\n")
        |> parser.parse(opts)

      {:error, message} ->
        raise message
    end
  end
end
bcardarella commented 5 months ago

I think in this case it is just going to be an interpolation step to produce an AST similar to what EEx.compile_string/2 is currently doing.

NduatiK commented 5 months ago

Exactly, the implementation shared above is basically a striped-down version of the EEx compiler. It's just the tokenizer function that needs to be improved to use the pre-parser you mentioned.

bcardarella commented 5 months ago

@NduatiK ah ok I'll check it out, thanks!

bcardarella commented 5 months ago

I forgot that String.Chars.to_string is just a protocol. I was able to quickly override the behavior:

defimpl String.Chars, for: Atom do
  def to_string(atom), do: Atom.to_string(atom)
end

but I'm getting the expected warning:

    warning: redefining module String.Chars.Atom (current version loaded from /Users/bcardarella/.asdf/installs/elixir/1.16.2/lib/elixir/ebin/Elixir.String.Chars.Atom.beam)
    │
  4 │   defimpl String.Chars, for: Atom do
    │   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    │
    └─ lib/live_view_native/stylesheet/rules_parser.ex:4: String.Chars.Atom (module)

It does solve our nil issue but this isn't the proper solution as I am pretty sure this is leaky

NduatiK commented 5 months ago

I think this might affect our user's code. It would not be responsible to add it.

bcardarella commented 5 months ago

It does, that’s why I highlighted it being leaky. It isn’t being considered for the fix