Refactor/async await #199
Refactor/async await #199
Conversation
src/blockstore.js
Outdated
| return setImmediate(() => { | ||
| callback(new Error('Not a valid cid')) | ||
| }) | ||
| throw new Error('Not a valid cid') |
There was a problem hiding this comment.
Needs to be async or users can't delete(cid).catch(console.log) this error.
There's quite a few places where this happens - we should decide if allowing library consumers to use promise syntax is important.
cc @ipfs/wg-js-core
There was a problem hiding this comment.
Good point - I think it's important, at least in the places where the documentation says it returns a Promise
There was a problem hiding this comment.
Marking things async creates a Promise. Best case scenario there's no actual async in the function, but the VM will still put the code after the promise onto the microtask queue which creates a small amount of latency. On hot codepaths this adds up as the Nearform performance analysis of our codebase showed. Our linting rules enforce require-await for this reason (and would cause the suggested change to fail linting).
I take the point about promise syntax - it's not great to have an API where some functions are async and other aren't, but we should not create artificial asynchronicity for the reason outlined above.
Maybe I'm wrong but I'd consider this module an internal API, quite different to js-ipfs itself so the inconsistency bothers me, but not to the point that we should make the code less efficient.
There was a problem hiding this comment.
I'm curious to know if await syncFn() has the same effect.
We're always going to call functions that may or may not be async with await right? - because we just don't know if the function will return a promise or not.
So, if await also forces async behaviour then is there a benefit to not using async?
There was a problem hiding this comment.
If the value of the expression following the await operator is not a Promise, it's converted to a resolved Promise.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await#Description
There was a problem hiding this comment.
Exactly why you shouldn't just await on everything 😁
FWIW the future-tech version of IPLD figures out if the return value of a function is thenable and resolves it if so (or at least it did in one iteration). Making everything async by default prevents these sorts of optimisations.
Anyway I've updated the PR, everything in the docs that claims to be async is now marked async.
src/lock-memory.js
Outdated
| @@ -11,41 +10,34 @@ const LOCKS = {} | |||
|
|
|||
| /** | |||
| * Lock the repo in the given dir. | |||
| * | |||
| * TODO | |||
There was a problem hiding this comment.
It's a mystery. Have removed it.
src/blockstore.js
Outdated
| return setImmediate(() => { | ||
| callback(new Error('Not a valid cid')) | ||
| }) | ||
| throw new Error('Not a valid cid') |
There was a problem hiding this comment.
Good point - I think it's important, at least in the places where the documentation says it returns a Promise
Co-Authored-By: Alan Shaw <[email protected]> Co-Authored-By: dirkmc <[email protected]>
|
@jacobheun any comments or can this be merged & released? |
|
@achingbrain v0.27.0 is out. |
Supersedes #189 and includes all of the wonderful work that @zcstarr did in that PR.
All async/await refactors that this relies on have been merged & released. I think this also now addresses all of the PR comments from #189 - can it be merged & released?