chore: make IPFS API static (remove api-manager) #3365
Conversation
Gozala
force-pushed
the
chore/kill-api-manager
branch
from
6452d8f
to
084b213
Compare
Gozala
force-pushed
the
chore/kill-api-manager
branch
from
9d89802
to
8e63ab4
Compare
There was a problem hiding this comment.
From the network stand point this looks good to me. I couldn't do an exhaustive review atm as this was quite big. Good work with all the types, I definitely need to do the same for libp2p.
Just left some minor observations I found and I will leave a more design heathy review for @achingbrain
| config.Bootstrap.push(multiaddr.toString()) | ||
| } | ||
|
|
||
| await repo.config.set(config) | ||
| await repo.config.replace(config) |
There was a problem hiding this comment.
why the change from set to replace ?
There was a problem hiding this comment.
Because interface of set is set(path,:string value:any) and replace has replace(value:Config). Later can ensure that what you pass is what's expected former can not. I also did not wanted to do the set overload typing because they tend to have issues.
There was a problem hiding this comment.
I appreciate that a change like this will touch a lot of files but if we are to merge changes like this in a timely fashion it's vitally important to restrict the scope of the work being done.
There seem to be a lot of internal refactors here that could have been done as separate PRs which would reduce the cognitive load on the reviewers and also give everyone involved a bit more confidence that the changes do not introduce regressions in poorly tested areas of the codebase.
| @@ -59,13 +59,11 @@ module.exports = ({ bitswap }) => { | |||
| * @typedef {object} BitswapStats - An object that contains information about the bitswap agent | |||
| * @property {number} provideBufLen - an integer | |||
| * @property {CID[]} wantlist | |||
| * @property {string[]} peers - array of peer IDs as Strings | |||
| * @property {CID[]} peers - array of peer IDs | |||
There was a problem hiding this comment.
See docs comment, peer IDs should be strings consistent with ipfs.swarm.peers
There was a problem hiding this comment.
Current implementation is not and docs / types were incorrect. And I have submitted #3389 to address that separately.
| @@ -0,0 +1,28 @@ | |||
| import CID from 'cids' | |||
There was a problem hiding this comment.
Why are there .ts files being added? The agreed approach for this project is to generate types from JSDoc comments.
There was a problem hiding this comment.
I think we talked about it. This are interface definitions for things we claim users can provide own implementations for. I think it would make sense to migrate some e.g. to https://github.com/ipfs/interface-datastore/ and others might need a new home. However it made most sense to me to put them here first and migrate them elsewhere afterwards.
This makes it possible to specify what interface option like bitswap, blockService, ipld, repo etc have to implement to qualify. Furthermore this dramatically increased type coverage. I think what we'd need to do in ipfs-bitswap, ipfs-block-service etc... is say that they @implements {BitSwap} etc... That way
-
We code against the interface not against our own implementation. We already use some internal properties that aren't part of the interface which implies that interface we have has gaps and we should probably rethink interface.
-
Other implementations could be coded against interface and
@implements {X}will enforce compatibility, as opposed to doing it against markdown documents that are (, as it turns out) out of sync.
So I don't think it's an alternative to generating types from code. It's complementary and forces us to uphold interface contract we our selfs defined.
| /** | ||
| * @private | ||
| * @param {Service<any, any>} service | ||
| * @returns {never} |
There was a problem hiding this comment.
Can this be @returns {void}? Then apparently we don't need the eslint-disable-next-line - https://eslint.org/docs/rules/valid-jsdoc
There was a problem hiding this comment.
No it needs to be never because it can never return and by consequence can be used e.g. to here:
const div = (a:number, b:number) : number => {
if (b === 0) return panic() // never
return a / b
}Note that if panic returned void type checker would complain that return type is number|void and not a number. Whilenever|number is equivalent of number.
Here specifically it used in switch statements
| @@ -35,6 +35,7 @@ module.exports = ({ gcLock, pin, refs, repo }) => { | |||
| // Mark all blocks that are being used | |||
| const markedSet = await createMarkedSet({ pin, refs, repo }) | |||
| // Get all blocks keys from the blockstore | |||
| // @ts-ignore - TS is not aware of keysOnly overload | |||
There was a problem hiding this comment.
do we have an issue for this ?
There was a problem hiding this comment.
There is this thread ipfs/interface-datastore#38 and as I mentioned somewhere here I do intend to trickle down interface defs introduces here to various packages as followup work, which I hope will be a driving force to rethink some of the API choices made.
For this specifically it seems like having a separate query method would be simple and pragmatic solution, that does not require some advanced type magic that I'm trying to reduce as much as possible.
| this.gc = createGC({ gcLock, pin, refs, repo }) | ||
| this.stat = createStat({ repo }) | ||
| this.version = createVersion({ repo }) |
There was a problem hiding this comment.
maybe we should inline the code for these in this file reducing the boilerplate, could be done for other API classes also.
There was a problem hiding this comment.
Not sure I understand what you mean, could you please elaborate ?
There was a problem hiding this comment.
now that we have the wrapper classes maybe we dont need one file per function everywhere and could inline those functions inside the wrapper class file reducing the types boilerplate.
There was a problem hiding this comment.
I don't mind that, but would rather pursue as separate endeavour if we decide to do it.
Co-authored-by: Hugo Dias <hugomrdias@gmail.com>
im good with discussing this in the other issue.
I think we are saying the some thing, just by different words.
We should not block this PR because of this. Just create an issue to follow up.
Im good with this change.
Im good with these, just think we should has a general rule try to make types simpler even if that makes us write more characters. |
|
achingbrain
changed the title
|
@Gozala can you please resolve the conflicts on this PR? |
@achingbrain I would much rather do it after I've got tentative 👍, because each takes I had to do it it took significant effort as I basically need to discard all changes to So if it is not too much to ask, I would very much prefer to get this PR to the point where it looks good and then get it up to date once instead of every time something is pushed to master. If this sounds unreasonable, I'll do what you asked. |
|
you got my 👍🏼 to rebase and get this merged ASAP |
|
@Gozala you have a tentative 👍, please resolve the conflicts on this PR to stop it becoming stale. |
|
@hugomrdias could you please release ipfsd-ctl that includes the changes from ipfs/js-ipfsd-ctl#560 because now |
|
@achingbrain @hugomrdias I have merged in all the upstream changes & fixed all the new errors that type-checker has discovered now that previously untyped dependencies (like In the process I also discovered problem with I have not got around to updating all the import paths which I said I would. But if you two are ok with doing it as followup PR, I could just refactor all of the typings as per #3413 |
👍🏼 |
|
@Gozala can you please create issues for all the follow-up work that needs to be done |
- api-manager is gone.
- There is now Storage component that is glorified `{keychain, repo, peerId}` tuple, that one can `async start` and it takes care of repo bootstrapping (which `init` used to do). As per discussion with @achingbrain `init` was mostly there for historical reasons.
- There is now `Network` service component that is glorified `{peerId, libp2p, bitswap}` tuple.
- All components that depended upon `libp2p` or `peerId` depend on `network` service now.
- They can do `await network.use(options)` and get tuple when node is started (if it is starting or started) or an exception if start has not been initiated.
- This way IPFS node is no longer mutated and APIs that depend on node been started throw if called before start.
- lot of TS typings were added to be able to make this changes with more confidence.
- Set of interfaces were added for things like datastore, repo, bitswap
- create can be passed implementations and it's useful to decouple interface from a concrete implementation.
- We had no types for those and it helped having interfaces to increase coverage and enable making these changes.
> I would like to migrate those to other repos, but doing it as would be more effective than having to coordinate changes across many repos.
- circular dependencies between pinmanager and dag APIs are resolved.
Co-authored-by: Alex Potsides <alex@achingbrain.net>
Co-authored-by: Hugo Dias <hugomrdias@gmail.com>
This pull request changes of how things are wired internally, to make it simpler and cleaner. Below is the high level overview of the changes:
{keychain, repo, peerId}tuple, that one canasync startand it takes care of repo bootstrapping (whichinitused to do). As per discussion with @achingbraininitwas mostly there for historical reasons.Networkservice component that is glorified{peerId, libp2p, bitswap}tuple.libp2porpeerIddepend onnetworkservice now.await network.use(options)and get tuple when node is started (if it is starting or started) or an exception if start has not been initiated.@achingbrain @vasco-santos please be vigorous. Tests end up catching more bugs than I could have anticipated & I'm bit afraid there might be some that slipped.