Skip to content

fix(migrations): properly replace imports across files#58414

Closed
pkozlowski-opensource wants to merge 1 commit intoangular:mainfrom
pkozlowski-opensource:output_migration_import_replace_fix
Closed

fix(migrations): properly replace imports across files#58414
pkozlowski-opensource wants to merge 1 commit intoangular:mainfrom
pkozlowski-opensource:output_migration_import_replace_fix

Conversation

@pkozlowski-opensource
Copy link
Member

This change fixes a bug where the output migration was interacting with the InputManager utility in the way that was resulting in incorrect import replacements.

The fix consists of making sure that a new ImportManager instance is created for each and every file containing @output declarations.

This change fixes a bug where the output migration was interacting
with the InputManager utility in the way that was resulting in
incorrect import replacements.

The fix consists of making sure that a new ImportManager instance
is created for each and every file containing @output declarations.
@pullapprove pullapprove bot requested a review from devversion October 29, 2024 17:07
@angular-robot angular-robot bot added the area: migrations Issues related to `ng update`/`ng generate` migrations label Oct 29, 2024
@ngbot ngbot bot added this to the Backlog milestone Oct 29, 2024
@pkozlowski-opensource
Copy link
Member Author

@devversion @crisbeto I might be very well misusing ImportManager but sharing its instance across migrated files was accumulating import replacements resulting in broken code.

Open to improvement suggestions, thnx!

@pkozlowski-opensource pkozlowski-opensource 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 Oct 29, 2024
@devversion
Copy link
Member

@pkozlowski-opensource with the approach we took for the output migration; this PR makes a lot of sense to me. In the future, we still have the TODO/AI to investigate how we can improve the import manager for Tsurge funnel migrations

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

Yeah, I don't see a reason why we should be sharing the instance across files.

@pkozlowski-opensource pkozlowski-opensource 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 Oct 30, 2024
@alxhub
Copy link
Member

alxhub commented Oct 30, 2024

This PR was merged into the repository by commit db7ed20.

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

@alxhub alxhub closed this in db7ed20 Oct 30, 2024
alxhub pushed a commit that referenced this pull request Oct 30, 2024
This change fixes a bug where the output migration was interacting
with the InputManager utility in the way that was resulting in
incorrect import replacements.

The fix consists of making sure that a new ImportManager instance
is created for each and every file containing @output declarations.

PR Close #58414
@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 Nov 30, 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: migrations Issues related to `ng update`/`ng generate` migrations target: rc This PR is targeted for the next release-candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants