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

feat: add js-kubo-rpc-client #764

Merged
merged 28 commits into from
Oct 5, 2022

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Aug 19, 2022

.aegir.js Outdated Show resolved Hide resolved
@SgtPooki
Copy link
Member Author

@lidel I removed the USE_KUBO_JS env var and just passed as overrides to ipfsd-ctl now. should be good to go on this PR

@SgtPooki SgtPooki requested a review from lidel September 20, 2022 03:56
@achingbrain achingbrain self-requested a review September 20, 2022 17:11
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth adding a test that uses the kubo rpc client

lidel
lidel previously requested changes Sep 20, 2022
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iiuc test/*.spec.js still use js-ipfs-http-client for talking to Kubo? (e.g., controller one)
I think you need to either refactor them to use js-kubo-rpc-client instead, or duplicate, so we test both.

@SgtPooki
Copy link
Member Author

Good catch, i'm not sure how I missed that. Updated and added logging to ensure that we're using the module we think we're using.

@SgtPooki
Copy link
Member Author

Log of running DEBUG='ipfsd-ctl:*rpcModule*' npm run test &> test.log.txt test.log.txt

@SgtPooki
Copy link
Member Author

iiuc test/*.spec.js still use js-ipfs-http-client for talking to Kubo? (e.g., controller one) I think you need to either refactor them to use js-kubo-rpc-client instead, or duplicate, so we test both.

There are already tests using kubo via ipfsd-ctl's type: 'go' so I just updated controller.spec.js so that it's using kuboRpcModule now. That should be sufficient.. I don't think adding a new test will gain us anything right now because they're essentially the same packages.

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few small tweaks required

src/ipfsd-daemon.js Outdated Show resolved Hide resolved
src/ipfsd-client.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@achingbrain
Copy link
Member

achingbrain commented Sep 30, 2022

I've done the typescript conversion work in #777 (like #776 but without the changes from this PR).

I don't want to merge it off the bat as it'll stomp all over these changes - if it's not a totally outrageous request, could you try integrating kubo-rpc-client in a PR targeting the branch from #777 - it'll likely give us a bit more confidence over the types as the jsdoc stuff is pretty limited.

@SgtPooki SgtPooki changed the base branch from master to feat/convert-to-typescript October 3, 2022 22:26
package.json Outdated
@@ -152,13 +152,15 @@
"wherearewe": "^2.0.1"
},
"devDependencies": {
"@libp2p/interfaces": "^3.0.3",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without this, ipfs-core-types was failing on a lack of types from @libp2p/interfaces/events from the line import type { EventHandler } from '@libp2p/interfaces/events'

@SgtPooki SgtPooki changed the base branch from feat/convert-to-typescript to master October 3, 2022 23:01
@SgtPooki SgtPooki marked this pull request as draft October 3, 2022 23:06
@SgtPooki SgtPooki marked this pull request as ready for review October 3, 2022 23:06
@SgtPooki
Copy link
Member Author

SgtPooki commented Oct 3, 2022

tests seem to be stuck on this PR for some reason.. running them at https://github.com/SgtPooki/js-ipfsd-ctl/actions/runs/3178121664

@SgtPooki SgtPooki dismissed stale reviews from achingbrain and lidel October 4, 2022 17:08

changes made

@SgtPooki
Copy link
Member Author

SgtPooki commented Oct 4, 2022

@achingbrain CI is passing

@BigLep
Copy link

BigLep commented Oct 4, 2022

@SgtPooki : that's great.

@achingbrain : any concerns with merging? I want get @SgtPooki unblocked. If it's not ok to merge, can you lay out what the conditions for merging will be?

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a couple of small nits and it's good to go.

src/index.ts Outdated Show resolved Hide resolved
package.json Outdated
@@ -152,13 +152,15 @@
"wherearewe": "^2.0.1"
},
"devDependencies": {
"@libp2p/interfaces": "^3.0.3",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dep is added but doesn't appear to be used anywhere?

Suggested change
"@libp2p/interfaces": "^3.0.3",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When removing this, ipfs-core-types throws an error

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SgtPooki : a couple of thoughts:

  1. Do you want to paste in the error observed to aid in understanding or debugging?
  2. assuming getting this merged is blocking other work you're trying to do for the js-kubo-rpc-client, I think it's safe to merge and then we fast-followup if there's something deeper to address that @achingbrain calls out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushing up a change removing this to see if CI still passes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BigLep thanks for chiming in. I was keeping an eye out for the error. I did already comment on the file changes with the error. I was going to share it in the CI if seen there, but it looks like it's going to pass

@SgtPooki SgtPooki merged commit 9538c1d into ipfs:master Oct 5, 2022
@SgtPooki SgtPooki deleted the feature/add-js-kubo-rpc-client branch October 5, 2022 19:23
github-actions bot pushed a commit that referenced this pull request Oct 5, 2022
## [12.2.0](v12.1.1...v12.2.0) (2022-10-05)

### Features

* add js-kubo-rpc-client ([#764](#764)) ([9538c1d](9538c1d))
@github-actions
Copy link

github-actions bot commented Oct 5, 2022

🎉 This PR is included in version 12.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/intermediate Prior experience is likely helpful P1 High: Likely tackled by core team if no one steps up released status/in-progress In progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add kuboRpcModule support to ipfs/js-ipfsd-ctl
4 participants