Skip to content

refactor(migration): support local string tokens for inject migration #62013

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

Closed

Conversation

JeanMeche
Copy link
Member

@JeanMeche JeanMeche commented Jun 11, 2025

The localTypeChecker allows us to at least support locally defined strings in addition to string literals.

Improves a bit the issue repported by #61982.

@pullapprove pullapprove bot requested a review from thePunderWoman June 11, 2025 17:19
@JeanMeche JeanMeche force-pushed the schematics/inject-string-var branch from 1ae5894 to f2c6539 Compare June 11, 2025 17:20
Copy link
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

LGTM!

@JeanMeche JeanMeche added target: patch This PR is targeted for the next patch release area: migrations Issues related to `ng update` migrations labels Jun 11, 2025
@ngbot ngbot bot modified the milestone: Backlog Jun 11, 2025
The `localTypeChecker` allows us to at least support locally defined strings in addition to string literals.
@JeanMeche JeanMeche force-pushed the schematics/inject-string-var branch from f2c6539 to 6ba7462 Compare June 11, 2025 17:25
@JeanMeche JeanMeche added the action: merge The PR is ready for merge by the caretaker label Jun 11, 2025
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

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

Minor nits to clarify that it is known that this support is only for locally declared symbols.

@@ -145,6 +145,35 @@ describe('inject migration', () => {
]);
});

it('should account for string tokens in @Inject()', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('should account for string tokens in @Inject()', async () => {
it('should account for locally declared string tokens in @Inject()', async () => {

@@ -885,3 +885,10 @@ function replaceParameterReferencesInInitializer(

return result.join('this.');
}

function isStringType(node: ts.Expression, checker: ts.TypeChecker): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

I'd add a docblock here to mention this results in false negatives for imported declarations, which we considered acceptable

@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit 4040a6b.

The changes were merged into the following branches: main, 20.0.x

pkozlowski-opensource pushed a commit that referenced this pull request Jun 12, 2025
#62013)

The `localTypeChecker` allows us to at least support locally defined strings in addition to string literals.

PR Close #62013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker area: migrations Issues related to `ng update` migrations target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants