feat(core): add ng generate schematic to convert to inject #57056

crisbeto · 2024-07-19T11:52:59Z

Adds the new ng generate @angular/core:inject-migration schematic that will convert existing code from constructor-based injection to injection using the inject function. The migration also has a few options that should help reduce compilation errors.

This migration is slightly different than our usual ones in that it may have to update entire class or constructor declarations. We don't go through the ts.factory.update* APIs for this, because it can cause the entire declaration to be re-formatted. Instead, this migration tries to insert strings in a way that won't affect the user's formatting.

eneajaho

Hi @crisbeto
I saw that generics case is not supported, for example: ElementRef<HTMLDivElement>

Because inject() doesn't infer the type correctly (check this issue: #53894) I guess, we also need to pass the whole type as a generic to inject.

el = inject<ElementRef<HTMLDivElement>>(ElementRef<HTMLDivElement>)

eneajaho · 2024-07-19T12:12:42Z

packages/core/schematics/ng-generate/inject-migration/README.md

Suggested change

import { Component, Inject, Optional, inject } from '@angular/core';

import { Component, inject } from '@angular/core';

Right now it's not removing the unused imports. I can look into it for a follow-up since it would involve the code we use to manage imports across all the schematics.

@crisbeto when you look into this, feel free to consider adding this to the ngtsc import manager as we likely should use that at some point.

packages/core/schematics/ng-generate/inject-migration/README.md

devversion

LGTM overall. Just some minor comments

packages/core/schematics/ng-generate/inject-migration/README.md

packages/core/schematics/ng-generate/inject-migration/analysis.ts

devversion · 2024-07-19T12:28:21Z

packages/core/schematics/ng-generate/inject-migration/analysis.ts

You can also use the type checker to check if it's nullable— but maybe this is intended to be just AST based

I'll double check it, but my assumption was that it might not work if it can't resolve types coming from other files.

Hmm. It might end up being the same, if the type is defined outside of the file. Although inside files, it would be more correct. Seems minor to not bother if that's the case

packages/core/schematics/ng-generate/inject-migration/migration.ts

devversion · 2024-07-19T12:35:20Z

packages/core/schematics/ng-generate/inject-migration/migration.ts

why can't we use the raw text of TypeNode here already?

Because it's a bit of a mix of string manipulation and creating an AST.

I'm not sure I 100% follow, but it doesn't matter much, so LGTM

Re-reading your comment now and I think I understand the question. If I remember correctly, TS does some sanitization of identifiers. I was a bit afraid that it might mangle a TypeNode that looks something like Foo<Bar<Baz>>.

IIRC it only does this when using a "generated identifier" via e.g. ts.createUniqueIdentifier

packages/core/schematics/ng-generate/inject-migration/migration.ts

Adds the new `ng generate @angular/core:inject-migration` schematic that will convert existing code from constructor-based injection to injection using the `inject` function. The migration also has a few options that should help reduce compilation errors. This migration is slightly different than our usual ones in that it may have to update entire class or constructor declarations. We don't go through the `ts.factory.update*` APIs for this, because it can cause the entire declaration to be re-formatted. Instead, this migration tries to insert strings in a way that won't affect the user's formatting.

crisbeto · 2024-07-19T12:51:52Z

Good point @eneajaho, I've added it.

Thank you for the quick review @devversion 🎉

eneajaho

All license links in the top comment point to: https://angular.io/license
Should we move them to: https://angular.dev/license

crisbeto · 2024-07-19T12:57:07Z

I looked into changing the licenses a few weeks ago, but it turned out to be kinda huge so I decided to leave it for later: #56379

rainerhahnekamp · 2024-07-19T21:26:46Z

Apologies for the question, but could you clarify why we are getting a migrator?

Did I miss a discussion or statement indicating that inject is now the preferred approach?

The last information I saw suggested that we could use any method we preferred, and that there would be a wait for the outcome of the standardization process of the parameter decorators.

Since the inject vs. constructor topic is quite polarizing, it might be helpful to release a statement before this change is merged.

crisbeto · 2024-07-22T17:55:33Z

@rainerhahnekamp both constructor-based injection and inject are supported right now and will continue being supported in the foreseeable future. We're getting this migrator, because:

We've seen that there's similar things popping up both internally and externally so there's a clear need for it.
We had some requests for it internally so it made sense to just open-source this so that all Angular users can use it.
Given that standard decorators don't support function parameters, there may be projects out there that want to switch to inject so that they can turn off experimental decorators.

rainerhahnekamp · 2024-07-22T20:44:04Z

Thanks @crisbeto, that was the answer I was looking for!

atscott · 2024-07-22T21:39:48Z

This PR was merged into the repository by commit fab673a.

The changes were merged into the following branches: main

…7056) Adds the new `ng generate @angular/core:inject-migration` schematic that will convert existing code from constructor-based injection to injection using the `inject` function. The migration also has a few options that should help reduce compilation errors. This migration is slightly different than our usual ones in that it may have to update entire class or constructor declarations. We don't go through the `ts.factory.update*` APIs for this, because it can cause the entire declaration to be re-formatted. Instead, this migration tries to insert strings in a way that won't affect the user's formatting. PR Close angular#57056

angular-automatic-lock-bot · 2024-08-22T00:10:17Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

ngbot bot added this to the Backlog milestone Jul 19, 2024

angular-robot bot added the detected: feature PR contains a feature commit label Jul 19, 2024

crisbeto force-pushed the inject-migration branch from f21d1c1 to cd6744c Compare July 19, 2024 12:05

crisbeto requested a review from devversion July 19, 2024 12:06

crisbeto marked this pull request as ready for review July 19, 2024 12:06

eneajaho reviewed Jul 19, 2024

View reviewed changes

eneajaho mentioned this pull request Jul 19, 2024

🎉 deprecate convert-di-to-inject-migration when Angular official solution lands ngxtension/ngxtension-platform#461

Closed

devversion approved these changes Jul 19, 2024

View reviewed changes

crisbeto force-pushed the inject-migration branch from cd6744c to 5c689b1 Compare July 19, 2024 12:54

eneajaho reviewed Jul 19, 2024

View reviewed changes

devversion approved these changes Jul 19, 2024

View reviewed changes

crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 22, 2024

atscott closed this in fab673a Jul 22, 2024

josuedeleonaguilar mentioned this pull request Aug 2, 2024

[Snyk] Upgrade: , , , , , , , josuedeleonaguilar/FlaskAngularpublic#3

Open

mayankv87 mentioned this pull request Aug 4, 2024

[Snyk] Upgrade: , , , , , , , mayankv87/movies-app-2019#40

Open

angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 22, 2024

	import { Component, Inject, Optional, inject } from '@angular/core';
	import { Component, inject } from '@angular/core';

feat(core): add ng generate schematic to convert to inject #57056

feat(core): add ng generate schematic to convert to inject #57056

Uh oh!

Conversation

crisbeto commented Jul 19, 2024

Uh oh!

eneajaho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

devversion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

crisbeto commented Jul 19, 2024

Uh oh!

eneajaho left a comment

Choose a reason for hiding this comment

Uh oh!

crisbeto commented Jul 19, 2024

Uh oh!

rainerhahnekamp commented Jul 19, 2024

Uh oh!

crisbeto commented Jul 22, 2024

Uh oh!

rainerhahnekamp commented Jul 22, 2024

Uh oh!

atscott commented Jul 22, 2024

Uh oh!

angular-automatic-lock-bot bot commented Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants