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

chore: update ipfs (now with typing) dependencies #1337

Merged
merged 16 commits into from
Jun 2, 2021

Conversation

achingbrain
Copy link
Contributor

This follows on from #1194 with a few extra bits and pieces (can target that branch for merging if it's preferable):

  • Updates types
  • Removes multiformats in favour of blockcodec-to-ipld-format as it outputs the correct types
  • Swaps ipfs for ipfs-core - no need to pull down cli deps etc if they're not being used
  • Updates ipld-dag-cbor to the latest version
  • Depends on next for ipfs-* to get the latest rc

@oed
Copy link
Member

oed commented Apr 30, 2021

Thanks @achingbrain looking forward to having native ipfs typings!

Will let others review this PR in depth.

Depends on next for ipfs-* to get the latest rc

I assume this PR is a draft until new IPFS versions are release? :)

@oed oed requested review from ukstv and stbrody April 30, 2021 10:22
@achingbrain achingbrain marked this pull request as draft April 30, 2021 10:23
@achingbrain
Copy link
Contributor Author

Yes! Sorry, I should have opened it as a draft..

@achingbrain
Copy link
Contributor Author

The following commands in this repo now work for me:

$ npm install
$ npm run build
$ npm test

Please let me know if there's anything else I should be running..

@oed
Copy link
Member

oed commented Apr 30, 2021

What node version are you on @achingbrain ?

What errors are you seeing when running npm install?

@achingbrain
Copy link
Contributor Author

What node version are you on @achingbrain ?

  1. I tried 15 but saw errors related to peer dependencies caused by the change in how npm@7 handles them.

What errors are you seeing when running npm install?

None, like I said, the commands above work without error 😄

@oed
Copy link
Member

oed commented Apr 30, 2021

Oh, haha!

I misread 😹

@oed
Copy link
Member

oed commented Apr 30, 2021

You should also run npm run lint.

@achingbrain
Copy link
Contributor Author

There's one @ts-ignore that needs removing, just needs ipfs/js-ipfs#3660 landing.

@achingbrain
Copy link
Contributor Author

That's been merged & released as an rc, linting is happy now.

Copy link
Contributor

@stbrody stbrody left a comment

Choose a reason for hiding this comment

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

A few very minor comments but overall this looks really great. I'm excited to get this merged in.

I'd still like @ukstv to take a look as well to double-check me, especially around the serialization/deserialization stuff in pubsub.

packages/common/src/index.ts Outdated Show resolved Hide resolved
packages/core/src/__tests__/ipfs-util.ts Outdated Show resolved Hide resolved
packages/core/src/pubsub/pubsub.ts Outdated Show resolved Hide resolved
packages/ipfs-daemon/src/ipfs-daemon.ts Outdated Show resolved Hide resolved
@achingbrain
Copy link
Contributor Author

@stbrody I think I've removed all the outstanding ts-ignores now, as well as the double-serialization of the pubsub message.

I'm going to flesh out the test suite for blockcodec-to-ipld-format and publish a v1 but other than that please feel free to point out any bits of the API that look odd to you or other points of friction.

@achingbrain achingbrain marked this pull request as ready for review May 12, 2021 11:15
@achingbrain
Copy link
Contributor Author

Everything's been released so this should be good to go, pending anything further review uncovers.

Copy link
Member

@oed oed left a comment

Choose a reason for hiding this comment

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

lgtm :)

Comment on lines +19 to +20
import type { IPFS } from 'ipfs-core-types'
export type IpfsApi = IPFS
Copy link
Member

Choose a reason for hiding this comment

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

This is nice. We should probably just start importing this type everywhere in the furtue, rather than reexporting it :)

@@ -148,9 +142,8 @@ export class IpfsDaemon {
...configuration.ipfsEnableApi && {API: `/ip4/${configuration.tcpHost}/tcp/${configuration.ipfsApiPort}`},
...configuration.ipfsEnableGateway && {Gateway: `/ip4/${configuration.tcpHost}/tcp/${configuration.ipfsGatewayPort}`}
,
Announce: configuration.announceAddressList,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this passed to both config and libp2p.config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually wondering the same thing, it shouldn't be necessary to pass the list in both places.

@oed
Copy link
Member

oed commented Jun 2, 2021

We should make sure that all of these work:

$ npm install
$ npm run build
$ npm test
$ npm run lint
$ npm run docs

): Promise<void> {
// Don't want to swarm connect to ourself
const myPeerId = (await ipfs.id()).id
const filteredBootstrapList = bootstrapList.filter((addr) => {
return !addr.endsWith(myPeerId)
return !addr.getPeerId()?.endsWith(myPeerId)
Copy link
Contributor

Choose a reason for hiding this comment

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

@achingbrain, I pulled down this PR and updated all the ipfs deps in the various package.json files to use next. The tests all passed, but running ceramic daemon in a terminal causes this line to throw the error:

Connecting to peers found in 'https://raw.githubusercontent.com/ceramicnetwork/peerlist/main/testnet-clay.json'
(node:126315) UnhandledPromiseRejectionWarning: TypeError: addr.getPeerId is not a function
    at /home/spencer/js-ceramic4/packages/ipfs-topology/lib/ipfs-topology.js:105:33
    at Array.filter (<anonymous>)
    at IpfsTopology._forceBootstrapConnection (/home/spencer/js-ceramic4/packages/ipfs-topology/lib/ipfs-topology.js:103:53)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async IpfsTopology.forceConnection (/home/spencer/js-ceramic4/packages/ipfs-topology/lib/ipfs-topology.js:78:9)
    at async IpfsTopology.start (/home/spencer/js-ceramic4/packages/ipfs-topology/lib/ipfs-topology.js:81:9)
    at async Ceramic._init (/home/spencer/js-ceramic4/packages/core/lib/ceramic.js:257:13)
    at async Function.create (/home/spencer/js-ceramic4/packages/cli/lib/ceramic-daemon.js:124:9)
    at async Command.<anonymous> (/home/spencer/js-ceramic4/packages/cli/lib/bin/ceramic.js:34:5)

Copy link
Member

Choose a reason for hiding this comment

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

The multiaddress in the file from there are in string and needs to be converted into a Multiaddress instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that was it. Pushed a fix to this PR branch, seems to be working now. I was able to run a node on the clay testnet, create and update a stream, and then load that stream from the clay gateway

@stbrody
Copy link
Contributor

stbrody commented Jun 2, 2021

We should make sure that all of these work:

$ npm install
$ npm run build
$ npm test
$ npm run lint
$ npm run docs

Done. All working

@stbrody stbrody merged commit c9438bd into ceramicnetwork:develop Jun 2, 2021
@achingbrain achingbrain deleted the achingbrain/ipfs-w-types branch June 5, 2021 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants