js-ipfs dependency updates, now with full types (WIP) #1194

rvagg · 2021-04-03T06:46:15Z

This is against an RC of the ipfs suite of packages which have full typing compiled and exported. Using this to test the RC for now but hopefully this will be a helpful addition when complete!

@achingbrain the things that stand out here are:

The JSIPFS type in ipfs-http-client, defined as IPFS & { ipld: IPLD, libp2p: libp2p }, is maybe too broad? I'm not sure what to do with this for new HttpApi() and I see we even still @ts-ignore those calls in tests too.
Probably need some integration work between dag-jose and the latest multiformats update and probably needs Output of multiformats/legacy should be an interface-ipld-format multiformats/js-multiformats#67 to be done
IPFS.create() config { pubsub: { enabled: boolean } } did that go away or do we have typing wrong for enabled?

oed

Thanks @rvagg! Glad to see the ipfs types working better.

Probably need some integration work between dag-jose and the latest multiformats update and probably needs multiformats/js-multiformats#67 to be done

Right, the dag-jose codec has not been updated in quite a while. It could definitely use an update to more recent packages (multiformats, cids, cbor).

oed · 2021-04-03T06:58:21Z

packages/core/src/dispatcher.ts

+    const asCid = typeof cid === 'string' ? new CID(cid) : cid
+    const record = await this._ipfs.dag.get(asCid, { timeout: IPFS_GET_TIMEOUT })


Does the dag api require CID instances now?

yes, but actually probably shouldn't! ipfs/js-ipfs#3637

oed · 2021-04-03T06:59:18Z

packages/core/src/pubsub/pubsub.ts

@@ -71,7 +72,7 @@ export class Pubsub extends Observable<PubsubMessage> {
    return this.peerId$
      .pipe(
        mergeMap(async (peerId) => {
-          await this.ipfs.pubsub.publish(this.topic, serialize(message));
+          await this.ipfs.pubsub.publish(this.topic, uint8ArrayFromString(serialize(message)));


I suppose we need to do the invert on incoming messages?

@oed It's already present https://github.com/ceramicnetwork/js-ceramic/blob/develop/packages/core/src/pubsub/pubsub-message.ts#L89

oed · 2021-04-03T07:00:26Z

packages/ipfs-daemon/src/ipfs-daemon.ts

+                dht: {
+                    enabled: false,
+                    clientMode: !configuration.ipfsDhtServerMode,
+                    randomWalk: false,
+                },
+                pubsub: {
+                    // TODO: @rvagg is this gone?
+                    // @ts-ignore
+                    enabled: configuration.ipfsEnablePubsub,


Were we doing this wrong or did it change?

@vasco-santos can you answer this one? I can't get to the bottom of the change here. dht and pubsub have been lifted out of config to the top level in the libp2p config options. Was that always wrong here, or a recent change, or a problem with types?

This is still inside the config, so this should not really be changed. Were there type errors configuring inside the config property?

The in progress branch of js-ipfs with the types improvements also has it inside the config, so the types should be correct upstream: https://github.com/ipfs/js-ipfs/blob/chore/update-buffer-deps/packages/ipfs-core/src/runtime/libp2p-nodejs.js#L42

See ipfs-core/src/types.d.ts and how it pulls in Libp2pConfig rather than Libp2pOptions. I think this is the source of the problem but you'll have to confirm. ipfs/js-ipfs#3640

oed · 2021-04-03T07:01:40Z

packages/ipfs-topology/src/ipfs-topology.ts

@@ -52,7 +51,7 @@ export class IpfsTopology {
  ) {}

  async forceConnection(): Promise<void> {
-    const base: string[] = BASE_BOOTSTRAP_LIST[this.ceramicNetwork] || [];
+    const base: Multiaddr[] = BASE_BOOTSTRAP_LIST[this.ceramicNetwork] || [];


Is there a change needed to BASE_BOOTSTRAP_LIST as well?

Probably shouldn't be so restrictive, opened an issue @ ipfs/js-ipfs#3638

rvagg · 2021-04-03T10:48:03Z

@oed I'd like to have a go at updating dag-jose, it'll be good validation of work we've been doing around multiformats and codecs recently, and we want to get the types right (tho the multiformats/legacy connection to js-ipfs is currently not properly done). The rest of the issues here will have to use more expert help from @achingbrain who's been deep in the js-ipfs types and knows much more about the API changes over time than I.

oed · 2021-04-03T12:10:47Z

@rvagg sounds great! Let us know if you run into any problems along the way.

ukstv · 2021-04-05T10:44:33Z

packages/core/src/pubsub/pubsub.ts

@@ -71,7 +72,7 @@ export class Pubsub extends Observable<PubsubMessage> {
    return this.peerId$
      .pipe(
        mergeMap(async (peerId) => {
-          await this.ipfs.pubsub.publish(this.topic, serialize(message));
+          await this.ipfs.pubsub.publish(this.topic, uint8ArrayFromString(serialize(message)));


Guess, this to uint8array convertation should be put into serialise function @rvagg

ukstv · 2021-04-05T10:49:05Z

Concerned about dag-jose codec (does it work with types enabled?) and typed Multiaddr in bootstrap list. For the latter assumption was that strings are passed.

rvagg · 2021-04-26T09:41:58Z

rebased to master and added some minor changes, still a few open questions about the js-ipfs core types

rvagg · 2021-05-04T10:52:39Z

continued in #1337, because that's a better number, obviously! thanks @achingbrain

oed requested review from ukstv and stbrody April 3, 2021 06:55

oed reviewed Apr 3, 2021

View reviewed changes

ukstv reviewed Apr 5, 2021

View reviewed changes

ukstv marked this pull request as ready for review April 5, 2021 10:49

ukstv marked this pull request as draft April 5, 2021 10:49

rvagg mentioned this pull request Apr 10, 2021

[CoreTypes] Tracking protocol/w3dt-sudo#17

Open

77 tasks

This was referenced Apr 26, 2021

[Types] DAGAPI get requires CID but should also be string|Uint8Array? ipfs/js-ipfs#3637

Closed

[Types] SwarmAPI.connect requires a Multiaddr but libp2p.dial allows PeerId|Multiaddr|string ipfs/js-ipfs#3638

Closed

rvagg force-pushed the rvagg/ipfs-w-types branch from 6c8f096 to dc0859a Compare April 26, 2021 09:27

rvagg added 2 commits April 26, 2021 19:37

chore: update ipfs (now with typing) dependencies

42491b7

fixup! chore: update ipfs (now with typing) dependencies

4a7476b

rvagg force-pushed the rvagg/ipfs-w-types branch from dc0859a to 4a7476b Compare April 26, 2021 09:41

This was referenced Apr 26, 2021

[Types] ipfs-http-server HttpApi requires JSIPFS type ipfs/js-ipfs#3639

Closed

[Types] IPFS.create() options uses Libp2pConfig instead of Libp2pOptions ipfs/js-ipfs#3640

Closed

achingbrain mentioned this pull request Apr 30, 2021

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

Merged

rvagg closed this May 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

js-ipfs dependency updates, now with full types (WIP) #1194

js-ipfs dependency updates, now with full types (WIP) #1194

rvagg commented Apr 3, 2021

oed left a comment

oed Apr 3, 2021

rvagg Apr 26, 2021

oed Apr 3, 2021

ukstv Apr 5, 2021

oed Apr 3, 2021

rvagg Apr 26, 2021

vasco-santos Apr 26, 2021

vasco-santos Apr 26, 2021

rvagg Apr 26, 2021

oed Apr 3, 2021

rvagg Apr 26, 2021

rvagg commented Apr 3, 2021

oed commented Apr 3, 2021

ukstv Apr 5, 2021

ukstv commented Apr 5, 2021

rvagg commented Apr 26, 2021

rvagg commented May 4, 2021

		const asCid = typeof cid === 'string' ? new CID(cid) : cid
		const record = await this._ipfs.dag.get(asCid, { timeout: IPFS_GET_TIMEOUT })

js-ipfs dependency updates, now with full types (WIP) #1194

js-ipfs dependency updates, now with full types (WIP) #1194

Conversation

rvagg commented Apr 3, 2021

oed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Apr 3, 2021

oed commented Apr 3, 2021

Choose a reason for hiding this comment

ukstv commented Apr 5, 2021

rvagg commented Apr 26, 2021

rvagg commented May 4, 2021