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

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

Closed
wants to merge 1 commit into from

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