refactor(async): Query #120
Conversation
|
not included in this PR, I started on edit: moving the discussion of this issue to this draft PR #121 |
|
Since Query and the other internal classes aren't exported, wouldn't it be better to just make all of that async directly? It looks like a lot of the methods have been promisified and/or have async counterparts. Ideally we'd only promisify other libs that have not been migrated, and callbackify the dht public api. Since this is only for the Query migration I understand that it's internally used methods would need to be callbackified, but the other classes should be able to be fully async. |
|
I wasn't sure if the other classes were considered public APIs or not, with respect to breaking changes. Thanks for the clarification. I'll take a look at how much of the existing callback APIs are still in use in other parts of the package. |
|
This PR takes us across the bundlesize, but note that I did not introduce any additional deps here |
|
@jacobheun ready to go
|
|
ok, fixed a commit failing commitlint that was several commits back using interactive rebase. bleh |
There was a problem hiding this comment.
1 minor comment in regards to documenting throws in the jsdocs.
Keeping in mind that this is designed to be a phased approach to async, I think this looks good. There are definitely things that can be cleaned up in the code, especially with the switch to async, but that should be done as a separate effort to avoid blocking these iterative changes.
| callback(null, new PeerQueue(key)) | ||
| }) | ||
| static async fromPeerId (id) { | ||
| const key = await promisify(cb => utils.convertPeerId(id, cb))() |
There was a problem hiding this comment.
This should throw now right? Should we add @throws to the jsdocs? It will make it easier for anyone consuming to know that they should ensure they're handling it. If so, we should add this to the other methods that also throw.
There was a problem hiding this comment.
@jacobheun Thats correct. What is the correct way to document throws for async functions?
There was a problem hiding this comment.
Looking more through the jsdocs repo there's not really a correct way, unfortunately. Ideally we'd be able to do something like this, jsdoc/jsdoc#1467, to be explicit.
For now, either way we have to do some level of assumption looking at the docs. Since the function is async, a throw is just a promise rejection. So we either need to catch it, or .catch the promise.
I suppose it makes sense to leave it out for now until the docs can be explicit.
non-breaking async refactor for
QueryRunPathPeerQueuethis is intended to not introduce any changes in behavior
related #82