Skip to content

feat(migrations): ngclass directives to class bindings #62983

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.

Sign up for GitHub

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

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

feat(migrations): ngclass directives to class bindings #62983

wants to merge 1 commit into from
+1,411 −0

Conversation

aparzi
Copy link
Contributor

@aparzi aparzi commented Aug 4, 2025

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.dev application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Added Migration Feature to migrate ngClass to class

Issue Number: #61661

What is the new behavior?

It'll migrate ngClass to class bindings to follow the style guide Prefer class over ngClass

Now

<div [class.admin]="isAdmin" [class.dense]="density === 'high'">

Before

<div [ngClass]="{admin: isAdmin, dense: density === 'high'}">

Does this PR introduce a breaking change?

  • Yes
  • No

@pullapprove pullapprove bot requested a review from mmalerba August 4, 2025 12:20
@angular-robot angular-robot bot added detected: feature PR contains a feature commit area: migrations Issues related to `ng update`/`ng generate` migrations labels Aug 4, 2025
@ngbot ngbot bot added this to the Backlog milestone Aug 4, 2025
@aparzi aparzi force-pushed the feat-ngclass-to-class branch from 81c69f2 to c96deb3 Compare August 4, 2025 12:21
@aparzi
Copy link
Contributor Author

aparzi commented Aug 4, 2025

Hi @JeanMeche,
this PR adds migration for ngClass to the class directive. There's definitely room for improvement; I'm looking forward to your feedback.

JeanMeche
JeanMeche reviewed Aug 4, 2025
View reviewed changes
JeanMeche
JeanMeche reviewed Aug 4, 2025
View reviewed changes
JeanMeche
JeanMeche reviewed Aug 4, 2025
View reviewed changes
JeanMeche
JeanMeche reviewed Aug 4, 2025
View reviewed changes
Copy link
Member

@JeanMeche JeanMeche left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for taking a look at this.
Let's start with aligning the test cases with the expectations.

We'll go through the implementation after.

@aparzi aparzi force-pushed the feat-ngclass-to-class branch from e17d83c to 7eed6fb Compare August 4, 2025 19:47
@aparzi aparzi requested a review from JeanMeche August 4, 2025 19:48
@aparzi
Copy link
Contributor Author

aparzi commented Aug 4, 2025

Hi @JeanMeche, I fix all your feedbacks. Except for related to the possible flag, I've only just currently update the relevant test case. Let me know if it's OK or if we want to proceed with a flag (if it's worth it).

JeanMeche
JeanMeche reviewed Aug 4, 2025
View reviewed changes
JeanMeche
JeanMeche reviewed Aug 4, 2025
View reviewed changes
JeanMeche
JeanMeche reviewed Aug 4, 2025
View reviewed changes
@aparzi aparzi force-pushed the feat-ngclass-to-class branch from 7eed6fb to fa20d78 Compare August 4, 2025 23:12
@aparzi
Copy link
Contributor Author

aparzi commented Aug 4, 2025

@JeanMeche
Updated PR with --migrate-space-separated-key the test cases seem to work. I just need to capture the option from the terminal, then we should be good to go.

@aparzi aparzi force-pushed the feat-ngclass-to-class branch from fa20d78 to c2a775c Compare August 5, 2025 07:54
@aparzi aparzi requested a review from JeanMeche August 5, 2025 07:54
@aparzi
Copy link
Contributor Author

aparzi commented Aug 5, 2025

@JeanMeche I updated this PR with custom option

@JeanMeche JeanMeche changed the title Feat ngclass to class feat(migrations): ngclass directives to class bindings Aug 5, 2025
JeanMeche
JeanMeche reviewed Aug 5, 2025
View reviewed changes
...ges/core/schematics/migrations/ngclass-to-class-migration/ngclass-to-class-migration.spec.ts Outdated
);
});
});

Copy link
Member

Choose a reason for hiding this comment

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

We're missing tests that ensure we remove the unused import.

Copy link
Member

Choose a reason for hiding this comment

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

We're still missing those tests.

JeanMeche
JeanMeche reviewed Aug 5, 2025
View reviewed changes
packages/core/schematics/migrations/ngclass-to-class-migration/ngclass-to-class-migration.ts Outdated
Comment on lines 78 to 79
const templateVisitor = new NgComponentTemplateVisitor(typeChecker);
templateVisitor.visitNode(node);

const replacementsForClass: Replacement[] = [];
let replacementCountForClass = 0;

templateVisitor.resolvedTemplates.forEach((template) => {
const {migrated, changed, replacementCount} = migrateNgClassBindings(
template.content,
this.config,
);
Copy link
Member

@JeanMeche JeanMeche Aug 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Since the following is text based. We should ensure first that the component is importing NgClass or CommonModule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean specifically?

Copy link
Member

Choose a reason for hiding this comment

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

The NgClassCollector is looking at strings not directive usages. This isn't safe enough. We need to make sure the ngClass attributes are actually instances of the directive.

Copy link
Member

Choose a reason for hiding this comment

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

This still need to be addressed.

@aparzi aparzi force-pushed the feat-ngclass-to-class branch from c2a775c to 43d4f78 Compare August 5, 2025 23:07
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 6, 2025 •
edited
Loading

Deployed adev-preview for 739ea29 to: https://ng-dev-previews-fw--pr-angular-angular-62983-adev-prev-bjeua1yr.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

@aparzi aparzi force-pushed the feat-ngclass-to-class branch from 6036e1e to 6882761 Compare August 6, 2025 13:37
@aparzi aparzi requested a review from JeanMeche August 6, 2025 13:38
@aparzi aparzi force-pushed the feat-ngclass-to-class branch from 6882761 to b44f7bf Compare August 6, 2025 17:04
@aparzi
Copy link
Contributor Author

aparzi commented Aug 6, 2025

@JeanMeche
I updated the PR with the requested changes. I also fixed the file structure because it was incorrect and adjusted all the test cases.

@JeanMeche
Copy link
Member

We'll need a rebase to be able to run the tests with the CI.

@aparzi aparzi force-pushed the feat-ngclass-to-class branch from b44f7bf to a7eee2a Compare August 6, 2025 20:16
@aparzi
Copy link
Contributor Author

aparzi commented Aug 6, 2025

We'll need a rebase to be able to run the tests with the CI.

Done. branch is up to date with main

JeanMeche
JeanMeche reviewed Aug 6, 2025
View reviewed changes
@aparzi aparzi force-pushed the feat-ngclass-to-class branch 2 times, most recently from 3405a84 to 6acbb64 Compare August 6, 2025 21:27
@aparzi aparzi requested a review from JeanMeche August 6, 2025 21:28
@aparzi
feat(migrations): add migration to convert ngClass to use class
739ea29 
feat angular#61661 - add migration to convert ngClass to use class
@aparzi aparzi force-pushed the feat-ngclass-to-class branch from 6acbb64 to 739ea29 Compare August 6, 2025 21:29
@aparzi
Copy link
Contributor Author

aparzi commented Aug 8, 2025

Hi @JeanMeche, any news fot this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmalerba mmalerba Awaiting requested review from mmalerba

@JeanMeche JeanMeche Awaiting requested review from JeanMeche

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
adev: preview area: migrations Issues related to `ng update`/`ng generate` migrations detected: feature PR contains a feature commit
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.
2 participants
@aparzi @JeanMeche