Skip to content

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`/`ng generate` 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

@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 Jul 21, 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 area: core Issues related to the framework runtime area: migrations Issues related to `ng update`/`ng generate` 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.

4 participants