Closed wmertens closed 2 weeks ago
Thanks Wout!
Interesting to see that the finally
propagates both the result or error, I wasn't aware of that.
Two feedbacks:
@josdejong @wmertens
This will be a useful addition until there can be a migration to the native Promise library. I have added some tests in a new branch to confirm matching functionality with the native finally
implementation.
https://github.com/joshLong145/workerpool/tree/feat/add-promise-finally
@joshLong145 I added your commit to this branch
Thanks for the update Wout, it has been a while 😅 .
Two remarks:
.finally()
method in the README.md? There is a list with all methods there that can be extended..then
and .catch
from the two tests, the test still succeeds. So, I think the test should check whether the .then
and .catch
have been trigered before calling done
. It is valuable at least to verify the order in which the .then
, .catch
, and .finally
are triggered so these two tests make sense to me.finally
method neatly propagates either the success or failure, but this is not yet captured in a unit test. Can you add two unit tests for that?@josdejong @wmertens
PR has been updated with
finally
implementationPromise.test.js
Thanks @joshLong145 for the updates! This looks good.
I made three small comments in the unit tests, can you have a look at those?
Thanks @josdejong for the feedback. Changes have been made.
Thanks for the updates Josh!
Fixes #169
Thanks to https://stackoverflow.com/a/53327815/124416