-
-
Notifications
You must be signed in to change notification settings - Fork 639
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
fix: do not abort atoms on unmount #2627
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Preview in LiveCodesLatest commit: fe3fad5
See documentations for usage instructions. |
What do we want to do here? As far as I can tell, it's impossible to satisfy this test case. |
Yeah, it's the test case we will break. Just remove it? |
☝️ @Pinpickle |
4b42c97
to
282b434
Compare
@dai-shi sorry! I've pushed the fix now. I've kept the test to assert that there are no aborts on unmount so this behaviour doesn't get reverted :) |
025578d
to
fe3fad5
Compare
Oops, had a TS error. Fixed now! |
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.
LGTM
Thanks for your contribution!
@@ -119,7 +119,7 @@ describe('abortable atom test', () => { | |||
expect(abortedCount).toBe(1) | |||
}) | |||
|
|||
it('can abort on unmount', async () => { | |||
it('does not abort on unmount', async () => { |
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.
it('does not abort on unmount', async () => { | |
it('does not abort on unmount (#2627)', async () => { |
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 makes sense! Though I see you've already merged!
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.
Ah, I forgot to apply changes. 😓
Thanks @dai-shi! |
Related Bug Reports or Discussions
Fixes #2625
Summary
@dai-shi agreed that not aborting promises on unsubscription was the best solution here.
When added to the continuable/cancellable promise idiom, it makes quite a lot of sense. Only abort the promise if you can replace it with another one.
I added the behaviour to store and store2
Check List
pnpm run prettier
for formatting code and docs