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(core): properly remove imports in the afterRender phase migration #56524

Conversation

cexbrayat
Copy link
Member

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.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Before this commit, the migration was removing the AfterRenderPhase enum from the imports but not the comma, resulting in invalid code:

ts

import { , Directive, afterRender } from '@angular/core';

What is the new behavior?

This commit fixes this by using updateNamedImports and replaceNode instead of removeNode.

After:

ts

import { Directive, afterRender } from '@angular/core';

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@pullapprove pullapprove bot requested a review from devversion June 19, 2024 18:14
@angular-robot angular-robot bot added the area: core Issues related to the framework runtime label Jun 19, 2024
@ngbot ngbot bot added this to the Backlog milestone Jun 19, 2024
@JoostK JoostK added area: migrations Issues related to `ng update` migrations target: minor This PR is targeted for the next minor release labels Jun 19, 2024
Before this commit, the migration was removing the `AfterRenderPhase` enum from the imports but not the comma, resulting in invalid code:

ts
```
import { , Directive, afterRender } from '@angular/core';
```

This commit fixes this by using `updateNamedImports` and `replaceNode` instead of `removeNode`.

After:

ts
```
import { Directive, afterRender } from '@angular/core';
```
@cexbrayat cexbrayat force-pushed the fix/after-render-phase-migration-imports branch from 2a9dcd9 to 7c7ba66 Compare June 19, 2024 18:42
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.

I'm not a core code owner so probably my approval isn't sufficient, but LGTM and thanks for the fix!

@devversion devversion added PullApprove: disable action: merge The PR is ready for merge by the caretaker labels Jun 19, 2024
@devversion devversion removed their request for review June 19, 2024 20:53
@devversion
Copy link
Member

devversion commented Jun 19, 2024

Joost's review is certainly sufficient (regardless of the group), so marking this PR as merge ready 😄 Thanks for the fix!

@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: presubmit The PR is in need of a google3 presubmit labels Jun 19, 2024
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit 03a2acd.

The changes were merged into the following branches: main

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: core Issues related to the framework runtime area: migrations Issues related to `ng update` migrations PullApprove: disable target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants