Skip to content

Conversation

@pkozlowski-opensource
Copy link
Member

This change extends the effect API surface so effects can, optionally, return a cleanup function. Such function, if returned, is executed prior to the subsequent effect run.

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Mar 29, 2023
@pkozlowski-opensource pkozlowski-opensource added area: core Issues related to the framework runtime target: major This PR is targeted for the next major release labels Mar 29, 2023
@ngbot ngbot bot modified the milestone: Backlog Mar 29, 2023
@pkozlowski-opensource pkozlowski-opensource force-pushed the reactivity_signals_effects_return_value branch from f31ce00 to 4af6b21 Compare March 29, 2023 10:24
@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review March 29, 2023 10:45
@pkozlowski-opensource pkozlowski-opensource added the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 29, 2023
Copy link
Member

@JeanMeche JeanMeche Mar 29, 2023

Choose a reason for hiding this comment

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

nit: () => void|WatchCleanupFn seems superfluous, it's a union on identical types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wait, I've meant () => (void|WatchCleanupFn) - it is a function that can return another (cleanup) function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now, but longer-term we should think about these long-lived operations and error handling. What should happen if an effect or its cleanup function throws an error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Making a note for myself.

@pullapprove pullapprove bot requested a review from alxhub March 29, 2023 13:44
@pkozlowski-opensource pkozlowski-opensource force-pushed the reactivity_signals_effects_return_value branch from 4af6b21 to 7288009 Compare March 29, 2023 14:24
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

Reviewed-For: public-api

(pending the EffectCleanupFn fix)

@pullapprove pullapprove bot requested a review from dylhunn March 29, 2023 14:34
@GabrielBB
Copy link

For anyone wanting to see a real case scenario: https://stackblitz.com/edit/angular-pknifp?file=src%2Fmain.ts (shared on twitter by @lifekefunde)

Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

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

reviewed-for: fw-core, public-api

@pkozlowski-opensource pkozlowski-opensource force-pushed the reactivity_signals_effects_return_value branch from 7288009 to 32f9341 Compare March 29, 2023 16:52
@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 Mar 29, 2023
This change extends the effect API surface so effects can, optionally,
return a cleanup function. Such function, if returned, is executed
prior to the subsequent effect run.
@pkozlowski-opensource pkozlowski-opensource force-pushed the reactivity_signals_effects_return_value branch from 32f9341 to 53fdc46 Compare March 29, 2023 17:09
@atscott
Copy link
Contributor

atscott commented Mar 29, 2023

This PR was merged into the repository by commit 9c5fd50.

@atscott atscott closed this in 9c5fd50 Mar 29, 2023
@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 Apr 29, 2023
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 detected: feature PR contains a feature commit target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants