Skip to content
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

Interpolating nil results in empty string #59

Closed
bcardarella opened this issue Feb 13, 2024 · 11 comments · Fixed by #64
Closed

Interpolating nil results in empty string #59

bcardarella opened this issue Feb 13, 2024 · 11 comments · Fixed by #64
Assignees
Labels
bug Something isn't working

Comments

@bcardarella
Copy link
Contributor

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 bcardarella added the bug Something isn't working label Feb 13, 2024
@bcardarella bcardarella self-assigned this Feb 13, 2024
@AZholtkevych AZholtkevych moved this to Todo in LiveView Native Feb 14, 2024
@bcardarella
Copy link
Contributor Author

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
Copy link
Contributor

NduatiK commented Mar 24, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

NduatiK commented Mar 24, 2024

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
Copy link
Contributor

NduatiK commented Mar 24, 2024

@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
Copy link
Contributor Author

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
Copy link
Contributor

NduatiK commented Mar 24, 2024

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
Copy link
Contributor Author

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

@bcardarella
Copy link
Contributor Author

bcardarella commented Mar 24, 2024

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
Copy link
Contributor

NduatiK commented Mar 24, 2024

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

@bcardarella
Copy link
Contributor Author

bcardarella commented Mar 24, 2024 via email

bcardarella added a commit that referenced this issue Apr 12, 2024
bcardarella added a commit that referenced this issue Apr 12, 2024
bcardarella added a commit that referenced this issue Apr 12, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in LiveView Native Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants