Skip to content

fix(upgrade): handle output emitters when downgrading a component#60369

Closed
crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto:60366/output-fn
Closed

fix(upgrade): handle output emitters when downgrading a component#60369
crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto:60366/output-fn

Conversation

@crisbeto
Copy link
Copy Markdown
Member

@crisbeto crisbeto commented Mar 13, 2025

The DowngradeComponentAdapter adapter was assuming that all outputs are observables, but they can also be OutputEmitterRef.

Fixes #60366.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Mar 13, 2025
@angular-robot angular-robot bot added the area: upgrade Issues related to AngularJS → Angular upgrade APIs label Mar 13, 2025
@ngbot ngbot bot added this to the Backlog milestone Mar 13, 2025
crisbeto
crisbeto commented Mar 13, 2025
View reviewed changes
packages/upgrade/src/common/src/downgrade_component_adapter.ts Outdated
Copy link
Copy Markdown
Member Author

@crisbeto crisbeto Mar 13, 2025

Choose a reason for hiding this comment

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

I went with checking for OutputEmitterRef, rather than EventEmitter here, because technically any observable can be an @Output.

Copy link
Copy Markdown
Member

@JoostK JoostK Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Couldn't we just always pass the closure directly without the object wrapper. Rxjs observables support that so its subclass EventEmitter should too?

Copy link
Copy Markdown
Member Author

@crisbeto crisbeto Mar 13, 2025

Choose a reason for hiding this comment

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

We can, but I was under the impression that signature was deprecated. I might be misremembering though...

Copy link
Copy Markdown
Member Author

@crisbeto crisbeto Mar 13, 2025

Choose a reason for hiding this comment

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

Ah now I remember, I think the signature with three arguments is deprecated. The one with a single argument should be fine. I'll switch it over.

@crisbeto crisbeto requested a review from thePunderWoman March 13, 2025 12:22
@crisbeto
fix(upgrade): handle output emitters when downgrading a component
4b57f56 
The `DowngradeComponentAdapter` adapter was assuming that all outputs are observables, but they can also be `OutputEmitterRef`.

Fixes angular#60366.
thePunderWoman
thePunderWoman approved these changes Mar 13, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

LGTM

@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 Mar 13, 2025
@pkozlowski-opensource
Copy link
Copy Markdown
Member

pkozlowski-opensource commented Mar 13, 2025

This PR was merged into the repository by commit cd7c170.

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

pkozlowski-opensource pushed a commit that referenced this pull request Mar 13, 2025
@crisbeto @pkozlowski-opensource
fix(upgrade): handle output emitters when downgrading a component (#6…
9e1a103 
…0369)

The `DowngradeComponentAdapter` adapter was assuming that all outputs are observables, but they can also be `OutputEmitterRef`.

Fixes #60366.

PR Close #60369
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Apr 13, 2025

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 Apr 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JoostK JoostK JoostK left review comments

@thePunderWoman thePunderWoman thePunderWoman approved these changes

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: upgrade Issues related to AngularJS → Angular upgrade APIs target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

Output emitters are not properly supported in hybrid AngularJS application

4 participants

@crisbeto @pkozlowski-opensource @JoostK @thePunderWoman