-
-
Notifications
You must be signed in to change notification settings - Fork 614
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(core,query): handle edge cases with writeAtom returning a promise #701
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pmndrs/jotai/C62GeUBwB1xsEKy4xGPAd5DhAHF4 |
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. Latest deployment of this branch, based on commit 7036fae:
|
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.
Looks good, feel like it's missing a test.
src/core/store.ts
Outdated
flushPending() | ||
}) | ||
return | ||
return writePromise.then(() => writeAtomState(atom, update)) |
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.
Is it possible to write a test for this inside core?
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.
I don't know. I can try. Do you know what this does?
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.
Actually, I'm not sure if we need this, as now writeAtom returns a promise.
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.
What I meant, is if you can write a test inside core, for the edge cases - Not the particular line. I'm sorry for not being clear.
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.
Sorry, I wasn't clear. You made a good point, we don't have a test for this, even if you don't know what it does.
The reason we didn't have a test is because we didn't know the actual use case.
Now, I feel like this isn't necessary at all. I will remove it completely.
): void => { | ||
const [atomState] = wipAtomState(atom) | ||
if (promise) { | ||
atomState.w = promise | ||
} else { | ||
} else if (atomState.w === prevPromise) { |
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.
Could you explain this with 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.
LGTM, only one nitpick request. :-)
#689 changed
writeAtom
to return a promise. This is a follow-up PR to cover some edge cases.