-
Notifications
You must be signed in to change notification settings - Fork 27k
feat(core): effects can optionally return a cleanup function #49625
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
feat(core): effects can optionally return a cleanup function #49625
Conversation
f31ce00 to
4af6b21
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
4af6b21 to
7288009
Compare
alxhub
left a comment
There was a problem hiding this 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)
|
For anyone wanting to see a real case scenario: https://stackblitz.com/edit/angular-pknifp?file=src%2Fmain.ts (shared on twitter by @lifekefunde) |
dylhunn
left a comment
There was a problem hiding this 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
7288009 to
32f9341
Compare
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.
32f9341 to
53fdc46
Compare
|
This PR was merged into the repository by commit 9c5fd50. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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.