Export IPFS instance type

[Xmader]

solves #2945 (comment)

Usage

import IPFS from 'ipfs'

function doSomething (ipfs: IPFS) {
    // ...
}

// ...
doSomething(await IPFS.create())

[achingbrain]

@Gozala could you take a look at this please? I was under the impression #3359 resolved this issue.

[Gozala]

@Gozala could you take a look at this please? I was under the impression #3359 resolved this issue.

I think I have overlooked the fact that IPFS class does not make all the way to module.exports but instead just it’s create does:

js-ipfs/packages/ipfs-core/src/index.js

Lines 16 to 29 in c3c4607

	module.exports = {
	create: IPFS.create,
	crypto,
	isIPFS,
	CID,
	multiaddr,
	multibase,
	multihash,
	multihashing,
	multicodec,
	PeerId,
	globSource,
	urlSource
	}

[Gozala]

@Xmader would you mind instead changing ipfs-core/src/index so that IPFS ends up been a class and we don’t have to introduce a type here too ? I’m assuming @achingbrain is onboard with this.

[Xmader]

@Xmader would you mind instead changing ipfs-core/src/index so that IPFS ends up been a class and we don’t have to introduce a type here too ? I’m assuming @achingbrain is onboard with this.

I tried changing

js-ipfs/packages/ipfs-core/src/index.js

Lines 16 to 29 in c3c4607

	module.exports = {
	create: IPFS.create,
	crypto,
	isIPFS,
	CID,
	multiaddr,
	multibase,
	multihash,
	multihashing,
	multicodec,
	PeerId,
	globSource,
	urlSource
	}

to

module.exports = class extends IPFS {
  static create = IPFS.create
  static crypto = crypto
  static isIPFS = isIPFS
  static CID = CID
  static multiaddr = multiaddr
  static multibase = multibase
  static multihash = multihash
  static multihashing = multihashing
  static multicodec = multicodec
  static PeerId = PeerId
  static globSource = globSource
  static urlSource = urlSource
}

but it does not work,

because without @typedef, the generated d.ts file of ipfs/src/index only declares and exports the constructor type

export = IPFS;
declare const IPFS: typeof import("ipfs-core/src");
//# sourceMappingURL=index.d.ts.map

[achingbrain]

I'm not sure exporting the IPFS class as the default export from ipfs is a great idea because we export CID, crypto etc as properties of the default export, so they'd become static fields of the IPFS class which seems like the wrong place to put them.

We could export the class as another property on the default:

const { IPFS } = require('ipfs-core')

const ipfs = new IPFS(...)

..but we don't do that right now by design because the node needs to do some async work before it's ready to be used. We found so much copy pasting of:

const IPFS = require('ipfs')

const node = new IPFS(opts)

node.on('error', (e) => {
  // .. error handling
})

node.on('ready', () => {
  // ... real works starts here
})

..that we introduced the create factory function which is much simpler to use, can be wrapped in try/catch, etc:

const { create } = require('ipfs')

const node = await create(opts)

..so we'd be intentionally exporting something that isn't terribly useful and we don't want people to actually use just to satisfy the TypeScript compiler, which seems like a high price to pay conceptually and in user confusion. If they're faced with a class and a factory method, which should they use? Why do they have to make this decision? Is one better than the other, etc.

Is there a way to just cause the return type of the factory method to end up in the generated typedef file with a JSDoc comment?

[Xmader]

Is there a way to just cause the return type of the factory method to end up in the generated typedef file with a JSDoc comment?

add this in packages/ipfs/src/index.js

/**
 * @typedef {ReturnType<typeof import('ipfs-core')['create']> extends Promise<infer U> ? U : never} IPFS
 */

which ends up to be this in the generated d.ts file

type IPFS = import("ipfs-core/src/components");

And it equals to

/**
 * @typedef {import('ipfs-core/src/components')} IPFS
 */

(this pull request)

[Gozala]

..so we'd be intentionally exporting something that isn't terribly useful and we don't want people to actually use just to satisfy the TypeScript compiler, which seems like a high price to pay conceptually and in user confusion. If they're faced with a class and a factory method, which should they use? Why do they have to make this decision? Is one better than the other, etc.

Not trying to argue, just presenting different point of view on this: There is nothing inherently wrong with doing new IPFS({ ... }) however that would require passing initialized repo and started network component, that one could do, but in most cases letting IPFS.create take care of all that is a better way to go.

Is there a way to just cause the return type of the factory method to end up in the generated typedef file with a JSDoc comment?

Problem is that IPFS class (or a type) isn't exposed by ipfs module so that TS user can't write a function annotation like doSomethingWithIPFS(ipfs: IPFS, ....) because they can't import IPFS here.

I thought just making require('ipfs') be that IPFS class was a better approach than what this pr does, because you don't wind up with something like:

import IPFS from 'ipfs`

const startApp (ipfs:IPFS) => {
}

const async main = () => {
   const ipfs = await IPFS.create(...)
   startApp(ipfs)
}

And instead do:

import IPFS from 'ipfs`
import { IPFS as IPFSType } from 'ipfs'

const startApp (ipfs:IPFSType) => {
}

const async main = () => {
   const ipfs = await IPFS.create(...)
   startApp(ipfs)
}

[Gozala]

Given that @achingbrain doesn't seem to like idea of exposing IPFS class, I think this is the best alternative we have. Lets do this for now and we can always improve this in the future.

[Xmader]

And instead do:

import IPFS from 'ipfs'
import { IPFS as IPFSType } from 'ipfs'

People can do

import IPFS from 'ipfs'
import type IPFSType from 'ipfs'

with this pr

[Gozala]

And instead do:

import IPFS from 'ipfs'
import { IPFS as IPFSType } from 'ipfs'

People can do

import IPFS from '.'
import type IPFSType from '.'

with this pr

Cool, that is better! But I do still think that it's kind of confusing & not sure that is even possible to express in JSDoc syntax. So personally I would prefer just exposing IPFS class and assume that users that choose to use new IPFS know what they are doing.

I also recognize that following is not ideal:

we export CID, crypto etc as properties of the default export, so they'd become static fields of the IPFS class which seems like the wrong place to put them.

But than again I don't personally find it to be so concerning and we even had a request to do #3053

It is also worth considering that if and when we switch to ESM we will no longer need to stick them into class statics as IPFS could be a default export and CID, crypto etc named ones.

[Gozala]

@Xmader, @achingbrain other alternative worth considering might be to stick IPFS class into module exports along with CID and others. That way it is possible to just do following:

import { IPFS } from "ipfs"

const startApp (ipfs:IPFS) => {
}

const async main = () => {
   const ipfs = await IPFS.create(...)
   startApp(ipfs)
}

But it's also possible to do what we do today:

const IPFS = require('ipfs')

const main = async () => {
  const ipfs = await IPFS.create()
}

And I think that would also align with potential ESM in the future.

[Xmader]

not sure that is even possible to express in JSDoc syntax.

It can.

const IPFS = require('ipfs')

/**
 * @typedef {import('ipfs')} IPFSType
 */

[achingbrain]

If I have test.ts like this (nb. importing ipfs-core and not ipfs since unless you need the daemon, the HTTP API and the CLI components you should be using ipfs-core and not ipfs):

// test.ts
import { create, IPFS } from "ipfs-core"

const startApp = async (ipfs:IPFS): Promise<void> => {
  await ipfs.id()
  // ... more code here
}

const main = async (): Promise<void> => {
   const ipfs = await create({})
   await startApp(ipfs)
}

Adding the typedef to /packages/ipfs-core/src/index.js like:

// ... more code here
const CID = require('cids')
const IPFS = require('./components')

/**
 * @typedef {import('./components')} IPFS     <-- added typedef
 */

module.exports = {
  create: IPFS.create,
// ... more code here

..results in a .d.ts like:

export type IPFS = import("./components");
export const crypto: typeof import("libp2p-crypto");
export const CID: any;
// ... more code here
export declare const create: typeof import("./components").create;
//# sourceMappingURL=index.d.ts.map

..test.ts then compiles happily.

But we should support requiring ipfs too for those who missed the blog post.

The ipfs module should just re-export all of the types from ipfs-core, including IPFS, though I'm having problems getting it to do that. It'll export everything from ipfs-core /except/ the IPFS type, which I can only export by renaming it (otherwise it complains about a duplicate identifier "IPFS"):

// index.js
const IPFSCore = require('ipfs-core')

/**
 * @typedef {import('ipfs-core/src').IPFS} RenamedIPFS
 */

module.exports = IPFSCore

// index.d.ts
export = IPFSCore;
declare const IPFSCore: typeof import("ipfs-core/src");
declare namespace IPFSCore {
    export { RenamedIPFS };
}
type RenamedIPFS = import("ipfs-core/src/components");

If I remove the name it gets called 'exports' which is less than ideal:

// index.js
const IPFSCore = require('ipfs-core')

/**
 * @typedef {import('ipfs-core/src').IPFS}
 */

module.exports = IPFSCore

// index.d.ts
export = IPFSCore;
declare const IPFSCore: typeof import("ipfs-core/src");
declare namespace IPFSCore {
    export { exports };
}
type exports = import("ipfs-core/src/components");

If I go in and manually edit .d.ts file to change the name, it seems to work and I can compile test.ts with the import changed to from ipfs-core to ipfs:

export = IPFSCore;
declare const IPFSCore: typeof import("ipfs-core/src");
declare namespace IPFSCore {
    export { IPFS };
}
type IPFS = import("ipfs-core/src/components");

Any ideas how to get ipfs to generate this .d.ts file or otherwise to just export all of the types from ipfs-core @Gozala @Xmader ?

[Xmader]

@achingbrain

Add // @ts-ignore in /packages/ipfs-core/src/index.js

const IPFS = require('ipfs-core')

// @ts-ignore duplicate identifier
/** @typedef {import('ipfs-core/src/components')} IPFS */

module.exports = IPFS

The .d.ts file can be generated perfectly.

export = IPFS;
declare const IPFS: typeof import("ipfs-core/src");
declare namespace IPFS {
    export { IPFS };
}
type IPFS = import("ipfs-core/src/components");
//# sourceMappingURL=index.d.ts.map

[Xmader]

By the way, people can replace ipfs with ipfs-core using npm i ipfs@npm:ipfs-core, so that it doesn't need to update the require name

[Xmader]

Is that OK to export IPFS type under export default as well, in ipfs-core?

// packages/ipfs-core/src/index.js

/**
 * @typedef {import('./components')} IPFS
 * @typedef {IPFS} default
 */

generated d.ts file

export type IPFS = import("./components");
type _default = import("./components");
export default _default;
export const crypto: typeof import("libp2p-crypto");
export const isIPFS: any;
export const CID: typeof import("cids");
// ... more code here
export declare const create: typeof import("./components").create;
//# sourceMappingURL=index.d.ts.map

so that people can do

import type IPFS from 'ipfs-core'

const startApp = async (ipfs: IPFS): Promise<void> => {
  await ipfs.id()
  // ... more code here
}

[achingbrain]

Interesting. How about this:

// packages/ipfs-core/src/index.js

// ... other code
const IPFS = require('./components')

/**
 * @typedef {import('ipfs-core/src/components')} default
 */

module.exports = {
  create: IPFS.create,
// ... other code

Generates:

type _default = import("./components");
export default _default;
export const crypto: typeof import("libp2p-crypto");
export const isIPFS: any;
export const CID: typeof import("cids");
export const multiaddr: typeof import("multiaddr");
export const multibase: typeof import("multibase/src/");
export const multihash: any;
export const multihashing: any;
export const multicodec: any;
export const PeerId: typeof import("peer-id");
export const globSource: any;
export const urlSource: any;
export declare const create: typeof import("./components").create;
//# sourceMappingURL=index.d.ts.map

// packages/ipfs/src/index.js

const IPFS = require('ipfs-core')

/** @typedef {import('ipfs-core/src/components')} IPFS default */

module.exports = IPFS

Generates:

export = IPFS;
declare const IPFS: typeof import("ipfs-core/src");
declare namespace IPFS {
    export { IPFS };
}
/**
 * default
 */
type IPFS = import("./components");
//# sourceMappingURL=index.d.ts.map

Then:

import type IPFS from "ipfs"  // <--- can be "ipfs" or "ipfs-core"
import { create } from "ipfs" // <--- can be "ipfs" or "ipfs-core"

const startApp = async (ipfs:IPFS): Promise<void> => {
  await ipfs.id()
}

const main = async (): Promise<void> => {
   const ipfs = await create({})
   await startApp(ipfs)
}

No @ts-ignore required.

If having IPFS as a separate type import is preferable and @Gozala is happy with this approach we can get this merged ASAP.

[Xmader]

I don't know which approach is more straightforward, and can get people less confused
export default, export as a separate type, or both?

I personally prefer just export default.

[achingbrain]

My opinion's not that strong, I like not requiring @ts-ignore to make it work but I don't know if having a separate import type will violate some sort of convention. Any thoughts @Gozala or is this good to go?

[Gozala]

I do not want to block this, but the default export as a means to export type makes me a little uncomfortable because:

• I’m not exactly sure how all this is going to work when we have actual ESM modules.
• Overlapping namespace of types and implementations is just confusing & TS with all it’s configs and issues makes me worried it’s going to be source of pain.

I think it would be a lot simpler to just export IPFS named type from both files and avoid naming conflicts by giving a different name to local bindings (as suggested in the PR)

[Gozala]

I think it would be simpler to do this instead

Suggested change

	const IPFS = require('ipfs-core')
	/**
	* @typedef {import(‘ipfs-core/src/components’)} IPFS
	*/
	module.exports = require('ipfs-core')

[Xmader]

It does not work because @typedef {import('ipfs-core/src/components')} IPFS will not overlap onto the exports

Generated d.ts file:

declare const _exports: typeof import('ipfs-core/src')
export = _exports;
export type IPFS = import('ipfs-core/src/components');

We can't use

import type IPFS from "ipfs"

[Gozala]

@Xmader I realize that, but point I was trying to make is, having export type IPFS is just simpler and in the nutshell will give us:

import { IPFS, create } from "ipfs"

Furthermore I expect that as we adopt approach proposed in #3413 we might end up with IPFS interface that in the future could be exported in place of IPFS type.

That said, it's an @achingbrain call

[Xmader]

I’m not exactly sure how all this is going to work when we have actual ESM modules.

It can still work

packages/ipfs-core/src/index.js

// ...
const multihash = multihashing.multihash
const CID = require('cids')

/**
 * @typedef {import('./components')} default
 */

export { create } from './components'
export {
  crypto,
  isIPFS,
  CID,
  multiaddr,
  multibase,
  multihash,
  multihashing,
  multicodec,
  PeerId,
  globSource,
  urlSource
}

Generates: (not much differ from what we have now)

export { create } from "./components";
type _default = import("./components");
export default _default;
export const crypto: typeof import("libp2p-crypto");
export const isIPFS: any;
export const CID: typeof import("cids");
export const multiaddr: typeof import("multiaddr");
export const multibase: typeof import('multibase');
export const multihash: any;
export const multihashing: any;
export const multicodec: any;
export const PeerId: typeof import("peer-id");
export const globSource: any;
export const urlSource: any;
//# sourceMappingURL=index.d.ts.map

packages/ipfs/src/index.js

/**
 * @typedef {import('ipfs-core').default} default
 */
export * from 'ipfs-core'

Generates:

export * from "ipfs-core/src";
type _default = import("ipfs-core/src/components");
export default _default;
//# sourceMappingURL=index.d.ts.map

[Gozala]

@Xmader I end up creating another pull request #3447 to illustrate what I was suggesting. Please do not take it as a subversion to your efforts, your contributions and time are really valuable, it just felt that having two different options to compare pros & cons side by side seemed more effective way to get to a consensus.

[achingbrain]

Like I said, my opinion on:

import type IPFS from "ipfs"
import { create } from "ipfs"

vs

import { IPFS, create } from "ipfs"

is just not that strong. I think less verbosity is better in general but I'm happy to be talked around.

I’m not exactly sure how all this is going to work when we have actual ESM modules.

I don't think we should worry about that right now as that will almost certainly need to be released as a breaking change anyway.

We can go with #3447 as it doesn't make CID etc look like class fields of IPFS.

@Xmader I second @Gozala - thanks so much for taking the time to look in to this and to make so many changes to the PR, your contributions are very much appreciated.