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

[TwigComponent] Ignore "nested" for Alpine & Vue attributes #2328

Open
wants to merge 5 commits into
base: 2.x
Choose a base branch
from

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
src/TwigComponent/src/ComponentAttributes.php Outdated Show resolved Hide resolved
@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 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
Collaborator

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.

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