Skip to content

Optimization level 2 #11

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

Open
wants to merge 15 commits into
base: feat/ux-icons-draft-2
Choose a base branch
from

Conversation

smnandre
Copy link

@smnandre smnandre commented Feb 3, 2024

Transform the component('icon') into icon() twig_function

To discuss (and test in performance... this one should be big)

@kbond
Copy link
Owner

kbond commented Feb 3, 2024

Can you show in a test how this would work? I think I'm not quite following. Does this replace the need for both the ux_icon() function and twig component?

@smnandre
Copy link
Author

smnandre commented Feb 3, 2024

Twig compilation is done in 3 steps

public function compileSource(Source $source): string
    {
        try {
            return $this->compile($this->parse($this->tokenize($source)));
        } catch (Error $e) {

In broad terms the idea beeing, for a given template:

  • Tokenizer: Transform a the template content into a set of tokens
  • Parser: Transform tokens into Nodes, and let some "nodevisitors" traverse the Node tree
  • Compiler: Transform nodes into a PHP Class

And to implement the HTML syntax we added a PreLexer that change the template content just before the tokenizer handles it.


In this case

Twig template

<twig:Ui:Icon name="github" class="foo bar" />

Tokenizer step

The TwigComponent PreLexer does it jobs and transform the HTML syntax into a component function call

{{ component('Ui:Icon', {name: "github",  class="foo bar"}) }}

Then the original Lexer transform the source of the template into the TokenStream

Parser step

The Parser... transform the TokenStream into a ModuleNode, and call the NodeTraversers.

My special NodeVisitor traverse all the template nodes, and when it finds a component function with the name "UxIcon" it transforms it back to a ux_icon function.

At this point we works with Node, but let's say the Node looks as if the template contained:

{{ ux_icon("github", {class: "foo bar"}) }}

Compiler

In the generated Template class, instead of having something like this

echo $this->extensions['Symfony\UX\TwigComponent\Twig\ComponentExtension']->render("Icon", ["name" => "user", "class" => "foo bar"]);

We have something like this.

echo $this->env->getRuntime('Symfony\UX\Icons\IconRenderer')->renderIcon("user", ["class" => "foo bar"]);

--

So what does it change ? It bypass entirely the TwigComponent system.

So we win:

  • a lot of performance

And we lose:

  • the events

For now it works only with the HTMLSyntax with no embed content (the self-closing tags), but we should discuss if we want to allow that anyway..

It works with hard-coded icon name, and i need to check in more dynamic use cases...

@smnandre
Copy link
Author

smnandre commented Feb 3, 2024

Can we poke Ryan on this too ?

I love this idea/feature, but it is not required now, and better not lose time on it if you guys feel it's a bit too "aggressive" (or any other reason) :)

@kbond
Copy link
Owner

kbond commented Feb 3, 2024

I love this idea/feature, but it is not required now

Ok, from what I'm seeing this could be added (either a generic thing in twig-components) or in ux-icons w/o BC breaks?

@smnandre
Copy link
Author

smnandre commented Feb 3, 2024

Of course :) With the EventListener it should already be fast ... and (warning: i'm on a mac) I made some tests on my machine, and the slowest part for me seems to be... the cache :|

@kbond kbond force-pushed the feat/ux-icons-draft-2 branch 3 times, most recently from d083168 to f5f30a1 Compare February 10, 2024 17:16
@kbond kbond force-pushed the feat/ux-icons-draft-2 branch 2 times, most recently from 42b1923 to fd873c7 Compare March 7, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants