Skip to content

fix(migrations): resolve multiple structural issues with HttpClient migration#55557

Closed
crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto:fix-http-migration
Closed

fix(migrations): resolve multiple structural issues with HttpClient migration#55557
crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto:fix-http-migration

Conversation

@crisbeto
Copy link
Member

Fixes several issues with the HttpClient migration that showed up after I tried updating the Material docs site to the latest v18 release. Includes:

  • The migration was assuming that all Angular decorators have at least one argument. This led to a null pointer error that broke the v18 update process when I was testing it.
  • The migration incorrectly reimplemented the detection of classes with Angular decorators. This can cause code to be migrated incorrectly and doesn't handle cases like import aliases. I've switched it to use the existing tooling for detecting decorated classes.
  • The migration was trying to migrate directives, even though they don't support the imports field.
  • The migration was detecting TestBed.configureTestingModule calls using a raw string match which is very fragile and can be broken by the user's formatting.
  • There were syntax errors in the unit tests.
  • There were type checking errors in the unit tests, for example none of them were importing the Angular decorators that they were migrating.

There's more room for improvement, but this should resolve the most glaring issues without having to rewrite too much.

…igration

Fixes several issues with the `HttpClient` migration that showed up after I tried updating the Material docs site to the latest v18 release. Includes:
* The migration was assuming that all Angular decorators have at least one argument. This led to a null pointer error that broke the v18 update process when I was testing it.
* The migration incorrectly reimplemented the detection of classes with Angular decorators. This can cause code to be migrated incorrectly and doesn't handle cases like import aliases. I've switched it to use the existing tooling for detecting decorated classes.
* The migration was trying to migrate directives, even though they don't support the `imports` field.
* The migration was detecting `TestBed.configureTestingModule` calls using a raw string match which is very fragile and can be broken by the user's formatting.
* There were syntax errors in the unit tests.
* There were type checking errors in the unit tests, for example none of them were importing the Angular decorators that they were migrating.

There's more room for improvement, but this should resolve the most glaring issues without having to rewrite too much.
@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: rc This PR is targeted for the next release-candidate labels Apr 26, 2024
@crisbeto crisbeto requested a review from dylhunn April 26, 2024 08:15
Copy link
Member

@JeanMeche JeanMeche left a comment

Choose a reason for hiding this comment

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

thanks for the improvement

@crisbeto 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 Apr 26, 2024
@crisbeto crisbeto removed the request for review from dylhunn April 26, 2024 08:39
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit 91b1f24.

AndrewKushnir pushed a commit that referenced this pull request Apr 26, 2024
…igration (#55557)

Fixes several issues with the `HttpClient` migration that showed up after I tried updating the Material docs site to the latest v18 release. Includes:
* The migration was assuming that all Angular decorators have at least one argument. This led to a null pointer error that broke the v18 update process when I was testing it.
* The migration incorrectly reimplemented the detection of classes with Angular decorators. This can cause code to be migrated incorrectly and doesn't handle cases like import aliases. I've switched it to use the existing tooling for detecting decorated classes.
* The migration was trying to migrate directives, even though they don't support the `imports` field.
* The migration was detecting `TestBed.configureTestingModule` calls using a raw string match which is very fragile and can be broken by the user's formatting.
* There were syntax errors in the unit tests.
* There were type checking errors in the unit tests, for example none of them were importing the Angular decorators that they were migrating.

There's more room for improvement, but this should resolve the most glaring issues without having to rewrite too much.

PR Close #55557
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker target: rc This PR is targeted for the next release-candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants