-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
refactor: clean up retrieval from data transfer item #114
refactor: clean up retrieval from data transfer item #114
Conversation
Pull Request Test Coverage Report for Build 12183872816Details
💛 - Coveralls |
This PR builds off the work from #112, so that should be merged first so this can be rebased on top. |
a04ee7b
to
a2a80e8
Compare
Re-based the PR and cleaned it up a little, I think we're good to go like this. @rolandjitsu can I ask you for a review? |
I'll put this PR on hold until we merged the backport under #122, as this is likely to create conflicts. |
58fbca4
to
90b9336
Compare
@rolandjitsu I've rebased this PR on top of the fixes we backported from Since the backports included converting the method to use |
As for the advanced errors with details about the handle, I think we could handle (no pun intended) that as a separate feature. Since the original error messages are kinda broken, and I have not seen anyone complain about it I think we can safely assume it is not that important to users, or they just don't get these messages often (or at all). |
90b9336
to
5ad6a9b
Compare
Signed-off-by: Jon Koops <[email protected]>
5ad6a9b
to
20a6c2b
Compare
@rolandjitsu did another pass and processed your comments, could I ask you for another review? |
Sorry for the long wait. Got a bit busy at work. |
No worries, I know how it is :) |
What kind of change does this PR introduce?
Did you add tests for your changes?
If relevant, did you update the documentation?
Summary
Cleans up the code in the
fromDataTransferItem()
function so that it is fully type-safe and usesawait
everywhere for Promises.Does this PR introduce a breaking change?
No
Other information
Not relevant