Skip to content
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

Create js-kubo-rpc-client library #9125

Closed
9 of 10 tasks
Tracked by #107 ...
lidel opened this issue Jul 19, 2022 · 32 comments
Closed
9 of 10 tasks
Tracked by #107 ...

Create js-kubo-rpc-client library #9125

lidel opened this issue Jul 19, 2022 · 32 comments
Assignees
Labels
effort/weeks Estimated to take multiple weeks kind/enhancement A net-new feature or improvement to an existing feature kind/maintenance Work required to avoid breaking changes or harm to project's status quo starmaps

Comments

@lidel
Copy link
Member

lidel commented Jul 19, 2022

ETA: 2022-12-16

Part of #8959 – based on discussions from IPFS Thing 2022
GO counterpart: #9124

Current state

Kubo shares RPC client with JS-IPFS:

After renaming GO-IPFS to Kubo, this state of things is arguably a bad user experience:

  • js-ipfs and Kubo are joined at the RPC hip – unable to improve their respective RPC interfaces, and pretending to be interoperable:

Desired state

Ideally, we should have one Kubo client at https://github.com/ipfs/js-kubo-rpc-client and kubo-rpc-client on NPM.

Keeping go- prefix is important because we want consistency across languages,
and Kubo team will maintain go-kubo-rpc-client too.

TODO

Below is a broad strokes plan, feel free to adjust if something else makes more sense:

Followups

Below are good improvements, but since we didn't have them with ipfs-http-client, we're not requiring them here.

cc @2color @BigLep @achingbrain @SgtPooki

@lidel lidel added the kind/maintenance Work required to avoid breaking changes or harm to project's status quo label Jul 19, 2022
@SgtPooki SgtPooki self-assigned this Aug 18, 2022
@SgtPooki
Copy link
Member

SgtPooki commented Aug 18, 2022

@SgtPooki
Copy link
Member

SgtPooki commented Aug 18, 2022

Trying to generate documentation, but I can't seem to do so and can't find instructions at https://github.com/ipfs/js-ipfs


It seems like aegir doesn't support generating docs anymore (aegir docs command no longer exists), and they used to use typedoc, so I'm going to use https://github.com/TypeStrong/typedoc directly

Action is running right now: https://github.com/ipfs/js-kubo-rpc-client/actions/runs/2886171949

Updated the action, ipfs/js-kubo-rpc-client@cc2823b, and the site is now visible at https://ipfs.github.io/js-kubo-rpc-client/

@SgtPooki
Copy link
Member

Created PR to add js-kubo-rpc-client to ipfs/github-mgmt#43

@SgtPooki
Copy link
Member

SgtPooki commented Aug 19, 2022

Using personal NPM_TOKEN for publish js-kubo-rpc-client to npm automatically with github action via aegir until I get access to npm service account to create more permanent NPM_TOKEN

@SgtPooki
Copy link
Member

Unified CI should be enabled for js-kubo-rpc-client once protocol/.github#371 and ipfs/github-mgmt#43 are merged

@SgtPooki
Copy link
Member

Updated NPM_TOKEN to use automated token from service account

@SgtPooki
Copy link
Member

update https://github.com/ipfs/js-ipfsd-ctl to support both ipfsHttpModule and kuboRpcModule
(we will also rename binary package, but that is tracked separately in ipfs/npm-kubo#51)

Starting work on this now.

@SgtPooki
Copy link
Member

SgtPooki added a commit to SgtPooki/js-ipfsd-ctl that referenced this issue Aug 19, 2022
@SgtPooki
Copy link
Member

decide if js-kubo-rpc-client CI should reuse tests against Core APIs interface-ipfs-core

@lidel @BigLep @aschmahmann, who should I talk to about this? Which stakeholders make the final call for this item?

@SgtPooki
Copy link
Member

Created milestone for tracking each task: https://github.com/ipfs/js-kubo-rpc-client/milestone/1

@BigLep
Copy link
Contributor

BigLep commented Aug 23, 2022

decide if js-kubo-rpc-client CI should reuse tests against Core APIs interface-ipfs-core

@lidel @BigLep @aschmahmann, who should I talk to about this? Which stakeholders make the final call for this item?

@SgtPooki : We need to make sure that kubo (RPC server) and the js-kupo-rpc-client can interopoerate and that we don't break compatibility between them. We should take whatever tests we had in the past to bootstrap our efforts, but we no longer need to adhere to the "Core API" concept. Basically, I would write tests that invoke the js-kubo-rpc client directly, rather than through legacy "Core API" interfaces.

@SgtPooki
Copy link
Member

So it sounds like Kubo RPC Client should have it's own tests, stripped out from interface-ipfs-core. And we should only bring over the tests that are applicable for kubo's supported RPC calls.

I can make sense of that I think. Thanks

@BigLep
Copy link
Contributor

BigLep commented Sep 7, 2022

@SgtPooki and @lidel : question on ipfs/interop when this work is done. Will ipfs/interop assert that a kubo node and a js-ipfs node can continue to interact with each other (e.g., discover each other, transport data)? Will the way that ipfs/interop accomplish this by orchestrating the kubo node using the js-kubo-rpc-client and the js-ipfs node using ipfs-http-client?

@lidel
Copy link
Member Author

lidel commented Sep 7, 2022

I think for now, we want to do a surgical swap: drop-in replace of ipfs-http-client with kubo-rpc-client when the backend node is Kubo, but all existing tests remain and should pass.

@lidel
Copy link
Member Author

lidel commented Sep 22, 2022

Seems we are back to the underlying question if both ipfs-http-client and kubo-rpc-client should conform to the same JS Interface (Core API) or not.

My understanding is:

  • Iroh does not implement Kubo RPC at all, and there are no plans to do that.
    • The only interface other implementations use for benchmarking is HTTP Gateway.
  • One of the reasons why we were asked to rename go-ipfs to Kubo was to make it clear that RPC at /api/v0/ is Kubo-specific, and other implementations are free to go their own way, create own APIs.
  • Until now, Core API in JS-IPFS was forced to conform to whatever Kubo legacy existed.
    • Creation of kubo-rpc-client aims at removing this co-dependency, allowing Kubo RPC and JS-IPFS RPC to go their own ways.
    • Someone will ask: if these are not distinct creatures, then why did we create kubo-rpc-client in the first place?

At the same time we have years of legacy code that assumes both projects expose the same API.
Issue at hand with js-ipfsd-ctl is only a symptom of a problem that we will see over and over again.
Sadly, this is one of the reasons why Stewards tried to postpone the split and rename for so long – it is a ton of work and bad DX for preexisting users.

Some thoughts on minimizing the pain

Accounting for the legacy we live with, and being pragmatic (Ideally, we don't want to spend the rest of 2022 on this), I see three ways going forward, would appreciate sanity check from @achingbrain: how feasible (and painful) below are?

  1. Make kubo-rpc-client conform to Core API types from JS-IPFS.
    • We admit defeat for now, and keep them joined at a hip, but the roles are reversed and it is JS-IPFS wilggling the kubo-rpc-client from now.
    • Main win is that nobody has to refactor 999 projects (incl. dragon lairs like ipfs/interop)
  2. Create distinct types for kubo-rpc-client but keep using ipfsd-ctl
    • This may be the least painful way to do things, complexity and refactor-wise.
    • Migration plan: js-ipfsd-ctl exposes existing JS API as api but when node type is go, kubo-rpc-client is used for talking to Kubo, and api is replaced with a stubs that throw error asking people to use api from kuboRpc field instead.
      • Good: there is no silent breakage caused by api having different behavior when Kubo is used: this ensures GO people are forced to refactor their code to switch to kuboRpc field and Kubo types ( api field's types remain as Core API).
      • Bad: every internal app that uses ipfsd-ctl with Kubo daemon will have to be refactored (incl. dragon lairs like ipfs/interop)
  3. Create kubod-ctl
    • Same as 2, but with more work: adds maintenance burden of a separate package.
    • We could update ipfsd-ctl to throw error when go type is used, pointing people at this new package

@BigLep
Copy link
Contributor

BigLep commented Sep 23, 2022

Thanks for the comments here @lidel. A couple of thoughts.

  1. Please huddle to sync synchronously if it would help for getting decisions made. Maybe the js-ipfs triage time could be used (or afterwards since that is early for @SgtPooki ).
  2. This probably goes without saying, but just so it's clear what I care about, I don't care as much about having a JS Kubo RPC client that is named as such (although I do think it's ideal for users to have a name that's clear). My chief concern is allowing the Kubo team to own their RPC client without any considerations for js-ipfs and vice versa. Decoupling is the key. If in the process, it's just as easy to rename, then great - please do it. If renaming anything is causing pain, please feel free to skip. I want to optimize for future developer velocity more so than user clarity.

@achingbrain
Copy link
Member

In js-ipfs triage we have come up with a 4th option:

The neat trick of the Core API is that it's supported by js-ipfs in-process and both js-ipfs and Kubo over http. We have leveraged this to write the interop suite which is a) massively useful in proving the two implementations can talk to each other and b) about to cause significant type-related pain now that we want to allow the RPC APIs to diverge.

The 4th option is to just type the parts of the API that the interop suite uses, store those types in the interop suite and have it use them instead of the types from the Core API.

The theory is that both kubo-rpc-client and ipfs-http-client will have overlap with these types so tsc should be happy with them, and then both implementations (just Kubo really since js-ipfs is getting No New Features:tm:) are free to innovate in other areas of the API.

When Kubo breaks the API so badly that the interop suite types are no longer valid, hopefully we'll have the shiny new version of IPFS-on-js that will have it's own interop suite that maybe orchestrates kubo via some sort of API external to it a la go-libp2p-daemon's simple protobuf-based API and doesn't depend on the RPC client at all.

@SgtPooki
Copy link
Member

type the parts of the API that the interop suite uses, store those types in the interop suite and have it use them instead of the types from the Core API.

Excellent. The interop work living in the interop package makes the most sense and should keep us all unblocked for the longest period. Type augmentations should help keep the diverging types manageable.

SgtPooki added a commit to ipfs/js-ipfsd-ctl that referenced this issue Oct 5, 2022
* feat: add js-kubo-rpc-client

see ipfs/kubo#9125

* chore: update kubo-rpc-client package name

* fix: use cross-env to set USE_KUBO_JS

* fix: Remove USE_KUBO_JS env var

* chore(deps): remove cross-env

* docs(readme): update npm install cmds for using kubo-rpc-client

* chore: log whether kubo-rpc-client or ipfs-http-client is in use

* chore(tests): use kubo-rpc-client for commmunication with go controllers

* chore(lint): fix lint failure

* fix: addCorrectRpcModule returns the additionalOpts object

* docs(readme): correct kubo-rpc-client name

Co-authored-by: Alex Potsides <alex@achingbrain.net>

* fix: use correct logger namespace convention

* fix: move kubo-rpc-client to devDeps

* fix: support generics for Controller type

* chore(deps): use kubo-rpc-client@1.0.1

* feat!: convert to typescript

Converts this module to typescript

* chore: fix linting

* chore: fix tests

* chore: apply suggestions from code review

* fix: resolve issues with js-kubo-rpc-client in typescript

* fix: use kuboRpcModule for go in factory.spec.ts

* fix: broken build from bad merge

* chore: address PR comments

Co-authored-by: Alex Potsides <alex@achingbrain.net>
@tinytb tinytb added kind/enhancement A net-new feature or improvement to an existing feature effort/weeks Estimated to take multiple weeks labels Oct 21, 2022
@BigLep
Copy link
Contributor

BigLep commented Nov 19, 2022

FYI I made a "CI test that fails when a new command is added to Kubo and is not supported by kubo-rpc-client yet" as followup that isn' required for closing this issue since we didn't have that with the ipfs-http-client.

@SgtPooki
Copy link
Member

SgtPooki commented Dec 5, 2022

Note that the latest work to close out the last action item in this issue (ipfs/js-kubo-rpc-client#5) is being handled via pull request ipfs/js-kubo-rpc-client#83.

I'll be meeting with @hacdias tomorrow (2022-12-06) to go over some of these tests and confirm next steps and handoff requirements.

Note that the original interface tests were testing against go-ipfs@0.12, and so the done criteria for closing this issue does not include updating interface tests to 0.17.

ninja-edit: There are technically two last items as seen on the milestone tracking the closing of this issue: https://github.com/ipfs/js-kubo-rpc-client/milestone/1, but the "revert patch-only release changes" is trivial so was not mentioned in the original comment.

@SgtPooki
Copy link
Member

SgtPooki commented Dec 7, 2022

Latest actions required prior to handoff: ipfs/js-kubo-rpc-client#83 (comment)

@SgtPooki
Copy link
Member

Note that ipfs/js-kubo-rpc-client#83 has been merged!

@SgtPooki
Copy link
Member

We should be good to close this. will defer to @BigLep and @lidel

@BigLep
Copy link
Contributor

BigLep commented Dec 17, 2022

Thanks @SgtPooki for bringing this to this point. The done criteria are satisfied, thus I think it's reasonable to close this out. I know there are additional tasks to do to further harden js-kubo-rpc-client and those are in ipfs/js-kubo-rpc-client#56

Closing - thanks again!

@2color
Copy link
Member

2color commented Jan 13, 2023

With this issue closed, should we point all users of https://github.com/ipfs/js-ipfs/tree/master/packages/ipfs-http-client to this?

The NPM page could also use a notice: https://www.npmjs.com/package/ipfs-http-client

@achingbrain
Copy link
Member

ipfs-http-client is still used to connect to js-ipfs, so not all users, but it could use a message saying to use the kubo rpc client with kubo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/weeks Estimated to take multiple weeks kind/enhancement A net-new feature or improvement to an existing feature kind/maintenance Work required to avoid breaking changes or harm to project's status quo starmaps
Projects
No open projects
Archived in project
Development

No branches or pull requests

7 participants