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

refactor(common): deprecate ngStyle and ngClass #58860

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JeanMeche
Copy link
Member

@JeanMeche JeanMeche commented Nov 24, 2024

The overhead of those directives (additional bundle size, distinct import) doesn't outweighs the few additional cases it supports compared to compiler provided class and style bindings.

DEPRECATE: Both ngStyle and ngClass directives are deprecated, use the their direct bindings instead.

Also, documentation the migration for this will address #40623.


Note:
The native bindings support almost the same usecases at the exception of 2 :

  • They don't support Set
  • They don't support keys with multiple styles/classes separated with spaces.
  • Unit suffix for style (eg myStyles = { 'width.px': 100}) wouldn't be supported either.

The recommendation is to drop those space-separated-keys into separate ones and replace Sets with arrays.

@JeanMeche JeanMeche added this to the v19.1 candidates milestone Nov 24, 2024
@ngbot ngbot bot removed this from the v19.1 candidates milestone Nov 24, 2024
@pullapprove pullapprove bot requested a review from alxhub November 24, 2024 22:35
@JeanMeche JeanMeche changed the title common: deprecate ngStyle and ngClass refactor(common): deprecate ngStyle and ngClass Nov 24, 2024
@angular-robot angular-robot bot added the area: common Issues related to APIs in the @angular/common package label Nov 24, 2024
@ngbot ngbot bot added this to the Backlog milestone Nov 24, 2024
@JeanMeche JeanMeche force-pushed the deprecate-ngclass branch 2 times, most recently from ddf04fc to 2150246 Compare November 24, 2024 23:54
@angular-robot angular-robot bot added the detected: deprecation PR contains a commit with a deprecation label Nov 24, 2024
The overhead of those directives doesn't outweighs the few additions cases it supports compared to compiler provided `class` and `style` bindings.

DEPRECATED: Both `ngStyle` and `ngClass` directives are deprecated, use the respective direct bindings instead.
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Reviewed-for: public-api
Reviewed-for: fw-core

@DmitryEfimenko
Copy link

This is a deprecation of a very widely used feature. If it is to be deprecated, I'd like to see clear examples in the deprecation comments showing how to migrate code where key-value expression was used

@JeanMeche
Copy link
Member Author

JeanMeche commented Nov 25, 2024

@DmitryEfimenko key-value expressions are already supported by the existing bindings.

The numbers are quite important in Google's monorepo, ([ngClass] is used twice as much as [class].

Also a migration will be provided to migrate the cases where is it possible to migrate the templates but it will not be possible to migrate everything as there are some breaking cases that will require manuel updates. Alonge side the migration, we'll have a dedicate doc.

That being said, this will probably be a soft deprecation.

@destus90
Copy link
Contributor

@JeanMeche

What about unit suffixes in style? I found this link in this PR: #40623

Is it actual or not?

@JeanMeche JeanMeche added the target: minor This PR is targeted for the next minor release label Nov 26, 2024
@JeanMeche
Copy link
Member Author

This behavior works only for the directive but that can easily be worked arround without having to pull the whole directive.

@dhhyi
Copy link

dhhyi commented Nov 28, 2024

The native bindings support almost the same usecases at the exception of 2 :
...
They don't support keys with multiple styles/classes separated with spaces.

This second use case is probably the most common, when using Angular together with styling systems that are heavily based on utility css classes, like tailwind or bootstrap (to some extent). I am a bit worried about the additional bloat that will be necessary to split all used classes and repeat the condition.

Something not really uncommon like

<div [ngClass]="{ 'ring ring-pink-500 ring-offset-2 dark:ring-green-500' : condition }">...

Would need to become

<div [class]="{ 'ring' : condition, 'ring-pink-500' : condition, 'ring-offset-2' : condition, 'dark:ring-green-500' : condition }">...

Which is really not that great to look at and quite cumbersome to write.

Considering alternatives, you could of course introduce a helper class in the styling of the component and use @apply like this

.my-ring {
  @apply ring ring-pink-500 ring-offset-2 dark:ring-green-500;
}

And use the helper class like this

<div [class]="{ 'my-ring' : condition }">...

But this would contradict principles of tailwind to have utility and locality first (https://tailwindcss.com/docs/utility-first) and avoid abstractions (https://tailwindcss.com/docs/reusing-styles).

So I am concerned and have a few questions:

Is there another beautiful way of still keeping the awesome experience working with tailwind and Angular I haven't thought about?

Is it possible to enhance Angular to still support keys with multiple styles/classes separated with spaces?

@e-oz
Copy link

e-oz commented Nov 28, 2024

<div class="{{ condition ? 'ring ring-pink-500 ring-offset-2 dark:ring' : '' }}"></div>

@dhhyi
Copy link

dhhyi commented Nov 28, 2024

<div class="{{ condition ? 'ring ring-pink-500 ring-offset-2 dark:ring' : '' }}"></div>

That looks like a workaround, but not really like a legitimate replacement in my opinion. For one, having an obligatory dangling empty ternary path doesn't look really fancy 😅

For another reason, it's not really as powerful as controlling class with an object.

Considering the following example:

<div [ngClass]="{
  'ring ring-offset-2': isHighlighted(),
  'text-green-500 italic': isPremium(),
  'text-grey-50': !isPremium(),
}">...

where premium and highlighted represent two independent boolean states, you will have 4 case combinations that will be harder to represent in ternary without a significant amount of repetition. It will get even worse with more states.

My point - as abstract as it might sound - the way of setting multiple classes in object fashion by ngClass is an important feature that needs adequate replacement before discussing deprecating it.

@e-oz
Copy link

e-oz commented Nov 29, 2024

@dhhyi
This is a dangerous feature and should be avoided. It should never be used with multiple classes per condition.
Here is an example of why:
https://stackblitz.com/edit/stackblitz-starters-fnso2b?file=src%2Fmain.ts

When one of the conditions becomes false, the directive will remove the classes, enlisted for this condition, even if other conditions have these classes too. The result is unintuitive and not so predictable (depending on the positions of the rules).

That's why I think it should not be migrated to [class]. Even if it will, I will avoid it because of the reasons I mentioned.

@dhhyi
Copy link

dhhyi commented Nov 29, 2024

When one of the conditions becomes false, the directive will remove the classes, enlisted for this condition, even if other conditions have these classes too.

@e-oz
Oh wow, that's an interesting behavior.

But it's caused by undisciplined programmers, and taming them is a task for eslint.
This seems to be a job for a new rule that could be contributed to angular-eslint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: common Issues related to APIs in the @angular/common package detected: deprecation PR contains a commit with a deprecation target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants