-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(core): return promise from 'dag.resolve' #3131
Conversation
@achingbrain I'm working on adding tests and documentation. Wanted to validate my approach with you. Apart from adding tests in interface-ipfs-core/src/dag/resolve.js, would we also want to have tests in ipfs/test/core/dag.spec.js. What is the difference there? |
} | ||
|
||
} finally { | ||
if (release) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is release
defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
release
was supposed to be initiated by release = gcLock.readLock()
which i picked up from ./put.js. But now I dont think it makes sense here. So removed this block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in ipfs.dag.put
because adding blocks and pinning are async operations so gc could run between them which would result in the blocks being removed immediately after they were added.
We're only reading here, not writing so the lock is unnecessary.
The tests in The tests in /packages/ipfs/test cover functionality not in the core-api. Since the idea here is to document this method in the core api, tests should be in the |
6ba72d9
to
2480e12
Compare
Makes `ipfs.dag.resolve` behave the same when calling into core and over the http api. Adds documentation and interface tests for `ipfs.dag.resolve`. Supersedes #3131 Fixes #2962 BREAKING CHANGES: - `ipfs.dag.resolve` returns `Promise<{ cid, remainderPath }` instead of `AsyncIterator<{ value, remainderPath }>` - Previously the core api returned an async iterator and the http client returned a simple promise Co-authored-by: Tarun Batra <[email protected]>
Fixed by #3152 - I added you as a co-author so you'll appear in the contributors list. Thanks! |
Thanks @achingbrain. I planned to write tests and documentation but couldn't find time. #3152 looks great. Appreciate your help! |
No worries, sometimes the real world gets in the way - thanks for kicking it off. |
Makes `ipfs.dag.resolve` behave the same when calling into core and over the http api. Adds documentation and interface tests for `ipfs.dag.resolve`. Supersedes #3131 Fixes #2962 BREAKING CHANGES: - `ipfs.dag.resolve` returns `Promise<{ cid, remainderPath }` instead of `AsyncIterator<{ value, remainderPath }>` - Previously the core api returned an async iterator and the http client returned a simple promise Co-authored-by: Tarun Batra <[email protected]>
For #2962