Fix maxAge start on Promise settlement
#24
Conversation
krystofwoldrich
force-pushed
the
fixMaxAge
branch
from
345976b to
ffe9b2a
Compare
index.js
Outdated
| cache, | ||
| cacheKey | ||
| }); | ||
| if (!isNaN(maxAge)) { |
There was a problem hiding this comment.
It should fail loudly with a TypeError if maxAge doesn't pass Number.isSafeInteger.
There was a problem hiding this comment.
OK. Thanks, this if condition didn't make sense.
index.js
Outdated
| } | ||
| }; | ||
|
|
||
| if (result && result instanceof Promise) { |
There was a problem hiding this comment.
instanceof is not a good way to check for promises.
| if (result && result instanceof Promise) { | |
| if (result && typeof result.then === 'function') { |
There was a problem hiding this comment.
Why are you checking for a promise at all? You can just use Promise.resolve().
There was a problem hiding this comment.
Because Promise.resolve() returns a promise and I think it is better to keep the function working as it is now. That means when it gets a non-promise returning function it won't change it to a promise returrning function.
There was a problem hiding this comment.
The docs are pretty clear about it only accepting promise-returning functions though, even though we didn't actually validate that:
Promise-returning or async function to be memoized.
There was a problem hiding this comment.
Yes, you're right. I guess it's a matter of taste, I didn't want to change the behaviour if not necessary.
But if you look at it as a "bug", then it would make sense to change now.
There was a problem hiding this comment.
I've used the Promise.resolve() now.
index.js
Outdated
| cache.delete(cacheKey(arguments_)); | ||
| const result = fn.apply(this, arguments_); | ||
|
|
||
| let resultError = null; |
There was a problem hiding this comment.
| let resultError = null; | |
| let resultError; |
The implementation is no longer depending on mem implementation therefore the typying should not either.
Since clearing test is the only one with sync function its testing two thing at once. Therefore it needed to be fecaupled to test only the clear and new test needed to be added to test keeping sync functions sync.
krystofwoldrich
force-pushed
the
fixMaxAge
branch
from
8747b43 to
68ab0f5
Compare
|
@sindresorhus You can take a second look, I've made the changes you mentioned. |
fregante
left a comment
fregante
left a comment
There was a problem hiding this comment.
A bit late to the party, but I see a major issue and a couple of improvements
| try { | ||
| return await Promise.resolve(result); | ||
| } catch (error) { | ||
| resultError = error; |
There was a problem hiding this comment.
I think it's easier to set or clear the cache here, even if you have to duplicate the cache.set call, so you don't have to create temporary result and resultError variables
There was a problem hiding this comment.
I think this is only a matter of preference.
I see this as a more clear approach to have only one cache set.
| resultError = error; | ||
| throw error; | ||
| } finally { | ||
| cache.set(key, { |
There was a problem hiding this comment.
This means the cache is set AFTER the promise is fulfilled, which means these 2 calls will not be memoized:
void memFetch()
void memFetch()There was a problem hiding this comment.
To achieve both initial call memoization and move the start time to the fulfillment, it'd have to first store the call without expiration and then re-add it with the right expiration once it fulfills.
There was a problem hiding this comment.
I think if someone wants this behaviour there is mem that does exactly that. Altho not exactly, but close.
I'm not sure if this would not bring unexpected behaviour for p-memoize. Since it should be oriented on promise function and this is more not promise usecase.
There was a problem hiding this comment.
It's totally valid to start multiple promises at the same time though. I agree with @fregante that this is a regression.
There was a problem hiding this comment.
Since it should be oriented on promise function and this is more not promise usecase.
I don't think @fregante made it clear enough in that example, but those are async functions.
There was a problem hiding this comment.
OK, guys, I've created a new PR (#25) which solves this.
3 participants
This is PR base on issue #22.
I've tried to keep the dependency on mem at firt, but I've not found any solution where it would make sense. After I added the maxAge map here to the
p-memoizethere wasn't basically any reason to keep mem in here. Maybe just for theclearfunction but in my opinien it is cleaner to have herecacheStorethen to have dependency just for that purpose.The only change is in the maxAge, all other behaviour should be unchanged.
Alltho the code would be cleaner if we would create from every function -> async function but I would rather have a bit more complicated code and keep the behaviour.
Let me know what you think or if you see any other nice, cleaner solutions.
Fixes #22