Fix consecutive non awaited calls #25
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fregante
previously requested changes
Sep 18, 2021
index.js
Outdated
Comment on lines
31
to
37
| let resultError; | ||
| try { | ||
| return await Promise.resolve(result); | ||
| return await result; | ||
| } catch (error) { | ||
| resultError = error; | ||
| throw error; | ||
| } finally { |
There was a problem hiding this comment.
@sindresorhus Since we're capturing the error, I think we should use Promise.allSettled instead of try/catch/finally. This feels cleaner:
const [{reason}] = await Promise.allSettled([promise])
if (reason && !cachePromiseRejection) {
cache.delete(key);
} else if (maxAge) {
// Promise fulfilled, so start the timer
cache.set(key, {
data: promise,
maxAge: Date.now() + maxAge
});
}
return promise;There was a problem hiding this comment.
I haven't used allSettled before but it looks super clean in this case.
There was a problem hiding this comment.
Once added, p-memoize will also need to bump to a minimum of Node 12 and therefore ESM https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c
There was a problem hiding this comment.
I can handle the Node.js bumping and ESM stuff after this PR.
|
Thanks for fixing, @krystofwoldrich 👍 |
|
Thanks for the prompt comments and reviews. @sindresorhus and @fregante |
niksy
pushed a commit
to niksy/p-memoize
that referenced
this pull request
Nov 10, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on our discussion after merging PR #24 I'm opening a new one hopefully solving mentioned issues.