-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
feat(core): effects can optionally return a cleanup function #49625
Conversation
f31ce00
to
4af6b21
Compare
|
||
constructor(private watch: () => void, private schedule: (watch: Watch) => void) { | ||
constructor(private watch: () => void|WatchCleanupFn, private schedule: (watch: Watch) => void) { |
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.
} finally { | ||
setActiveConsumer(prevConsumer); | ||
} | ||
} | ||
|
||
cleanup() { | ||
this.cleanupFn(); |
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
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) |
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.