Skip to content

Conversation

smnandre
Copy link
Member

@smnandre smnandre commented Nov 3, 2024

Q A
Bug fix? yes
New feature? no
Issues Fix #1839
License MIT

Have lost time on Twig & the website 😓

Alternative implementation of #2325

Update: Improved __toString performance following @Kocal comments


Now all these attributes are directly rendered

Framework Prefix Code Example Documentation
Alpine.js x- <div x-data="{ open: false }" x-show="open"></div> Documentation Alpine.js
Vue.js v- <input v-model="message" v-if="show"> Documentation Vue.js
Stencil @ <my-component @onClick="handleClick"></my-component> Documentation Stencil
Lit @ <button @click="${this.handleClick}">Click me</button> Documentation Lit

Have lost time on Twig & the website 😓
@carsonbot carsonbot added Bug Bug Fix TwigComponent Status: Needs Review Needs to be reviewed labels Nov 3, 2024
@smnandre smnandre requested review from kbond and Kocal November 3, 2024 02:04
@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Nov 3, 2024
smnandre and others added 2 commits November 3, 2024 14:52
Co-authored-by: Hugo Alliaume <hugo@alliau.me>
@smnandre smnandre requested a review from Kocal November 3, 2024 14:05
@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Reviewed Has been reviewed by a maintainer labels Nov 3, 2024
Running scenario: Short attributes, empty rendered
  __toString() time: 0.00077986717224121 seconds
  __toString2() time: 0.00049495697021484 seconds
  Improvement: 36.533170284317%
Running scenario: Short attributes, partial rendered
  __toString() time: 0.00054383277893066 seconds
  __toString2() time: 0.00031709671020508 seconds
  Improvement: 41.692240245506%
Running scenario: Short attributes, full rendered
  __toString() time: 0.00030899047851562 seconds
  __toString2() time: 0.00012898445129395 seconds
  Improvement: 58.256172839506%
Running scenario: Long attributes, empty rendered
  __toString() time: 0.0038020610809326 seconds
  __toString2() time: 0.0026099681854248 seconds
  Improvement: 31.353859660124%
Running scenario: Long attributes, partial rendered
  __toString() time: 0.0032980442047119 seconds
  __toString2() time: 0.0022611618041992 seconds
  Improvement: 31.439311790646%
Running scenario: Long attributes, full rendered
  __toString() time: 0.00074219703674316 seconds
  __toString2() time: 0.00014185905456543 seconds
  Improvement: 80.886604561516%
@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Nov 3, 2024
@WebMamba
Copy link
Contributor

WebMamba commented Nov 4, 2024

I am sorry but I don't like the solution taken here. I like way more the initial solution taken here: #2325. I think we should not add code specific to another framework, and I don't think that we should ignore nested attributes without making them implicit for the user. I think we should focus on a solution like the initial solution where the user choose to ignore it implicitly.

@Kocal
Copy link
Member

Kocal commented Nov 4, 2024

What about #2325 (comment) then?
People will have to adapt their code to make it works with Twig Components?

I don't find that super friendly, it not obvious at all, and can be a source of frustration to the user even if it's documented (and document that you need to explicitly use a new syntax to disable nested attributes to makes your code works, feels like a loss to me)

@smnandre
Copy link
Member Author

smnandre commented Nov 5, 2024

Are we really considering there is real scenario where someone

  • has a component
  • has a child component in it named "x-on"
  • uses nested attributes

?

Regarding specific code, we have some for LiveComponent & Alpine already. On the other hand, opening a new char / syntax in the attributes will be a change with many more problems.

@Kocal
Copy link
Member

Kocal commented Nov 23, 2024

Thank you @smnandre for taking care of this regression.

@Kocal Kocal merged commit 9219cdf into symfony:2.x Nov 23, 2024
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug Fix Performance Status: Reviewed Has been reviewed by a maintainer TwigComponent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AlpineJS specific attributes are not rendered (@click and x-on:click)
4 participants