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] Cva in html_extra #2070

Closed
smnandre opened this issue Aug 15, 2024 · 6 comments · Fixed by #2144
Closed

[TwigComponent] Cva in html_extra #2070

smnandre opened this issue Aug 15, 2024 · 6 comments · Fixed by #2144

Comments

@smnandre
Copy link
Member

It seems the cva function will be merged into twig/html-extra (see twigphp/Twig#4006)

How do we handle the transition ?

  • Add a conflict with the future version of html-extra here seems not a good idea, as this would be blocking for future updates
  • Add a deprecation layer here ?
  • Ask people to require twig/html-extra ?
  • What do we do about the documentation ?
@Kocal
Copy link
Member

Kocal commented Aug 15, 2024

We should be careful here since the cva() definition changes from here and from twigphp/Twig#4006:

  1. actual: public function cva(array $cva): CVA
  2. Introduce CVA to html-extra twigphp/Twig#4006: public static function cva(array|string|null $base = null, ?array $variants = null, ?array $compoundVariants = null, ?array $defaultVariant = null): Cva

This first one takes a big array, where the second one takes advantage of Twig named arguments (and that's super nice! I prefer this syntax too), but, we should find a way to correctly handle the migration from UX to twig/html-extra for cva.

If twigphp/Twig#4006 is merged, it has no interest to keep CVA's code in UX and we will need to remove it, in a major version.

If release a new major version is not wanted - which I agree because it will affect all other UX packages -, I think we can do it smoothly by:

  1. makes symfony/ux-twig-component requires twig/extra-html
  2. implement a "bridge" in symfony/ux-twig-component to nicely migrate and warn the user about passing an array to cva
  3. trigger a deprecation, telling the user to manually requires twig/extra-html and pass named arguments
  4. tag a new minor version

Then, for a new major version, remove this deprecation layer and twig/extra-html dependency.

For the documentation, I believe we can do something like:

Since 2.2x, cva function has moved to twig/extra-html.
We recommend you to upgrade your cva usages to (...), since cva will be removed from UX Twig Components (...).

WDYT?

@smnandre
Copy link
Member Author

  1. makes symfony/ux-twig-component requires twig/extra-html

I'm not in favor of requiring an external package (this would need twig-extra too) ... to deprecate one

And that does not change the fact someone updating twig/extra-html will break its app, no ? Two functions cannot be registered with the same name i think.

@smnandre
Copy link
Member Author

@WebMamba renamed the twig/html-extra function html_cva to ease this transition / diminish any conflict risks (thank you for this)

So indeed, next steps here:

  • once html_cva is released on twig/html-extra: deprecate the cva function (and suggest the twig version)
  • next major release of TwigComponent: remove the cva function

@Kocal
Copy link
Member

Kocal commented Sep 1, 2024

@smnandre I'm not sure why you closed this issue, our cva function is still not deprecated right?

@smnandre
Copy link
Member Author

smnandre commented Sep 1, 2024

This issue was to decide the "what to do" .. and we did .. but let's reopen if you prefer to keep a "todo" !

@Kocal
Copy link
Member

Kocal commented Sep 1, 2024

Ah, yes ok, it makes sense!
I would prefer if we re-open this issue or directly create another one, so our plan is not lost 🙏🏻

@Kocal Kocal reopened this Sep 1, 2024
Kocal added a commit that referenced this issue Sep 9, 2024
…ersion (Kocal)

This PR was merged into the 2.x branch.

Discussion
----------

[Map] Adjust changelogs and fix `render_map` deprecated version

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT

<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - For new features, provide some code snippets to help understand usage.
 - Features and deprecations must be submitted against branch main.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

While working on #2070, I noticed Map's changelogs were still containing `Unreleased` instead of the version number.
I've also changed the version where `render_map` twig function have been deprecated, to match the reality (the 2.20 will be the next release!).

Commits
-------

06b248a [Map] Adjust changelogs and fix `render_map` deprecated version
@Kocal Kocal closed this as completed in e22484b Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants