Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Track: Typescript types support #2945

Closed
hugomrdias opened this issue Mar 24, 2020 · 45 comments
Closed

Track: Typescript types support #2945

hugomrdias opened this issue Mar 24, 2020 · 45 comments
Assignees
Labels
awesome endeavour Epic exp/wizard Extensive knowledge (implications, ramifications) required kind/tracking A meta-issue for tracking work P0 Critical: Tackled by core team ASAP topic/docs Documentation

Comments

@hugomrdias
Copy link
Member

hugomrdias commented Mar 24, 2020

We will use JSDoc to declare types for the top level API first and internally we will add types incrementally where we feel it adds value.

A TS config will be added to the repo to enable type declaration generation from JSDoc comments.

Type declaration tests will be added as we see fit and we will add a ts type check job to our CI.

Repo track list

Done = ✅
In Progress = 🚧
TODO = ⛔

Status Repo/PR Owner
aegir ipfs/aegir#671 ipfs/aegir#701 -
ipfs-utils ipfs/js-ipfs-utils#89 -
interface-datastore ipfs/interface-datastore#56 -
datastore-core ipfs/js-datastore-core#39 -
datastore-level ipfs/js-datastore-level#53 -
datastore-pubsub ipfs/js-datastore-pubsub#60 @achingbrain
datastore-fs ipfs/js-datastore-fs#62 -
ipfs-repo ipfs/js-ipfs-repo#275 @hugomrdias
ipfs-unixfs ipfs/js-ipfs-unixfs#114 ipfs/js-ipfs-unixfs#122 @achingbrain
ipfs-bitswap ipfs/js-ipfs-bitswap#261 @Gozala
is-ipfs ipfs-shipyard/is-ipfs#39 @achingbrain
ipns ipfs/js-ipns#106 @achingbrain
ipfs-block-service ipfs/js-ipfs-block-service#136 @achingbrain
cids multiformats/js-cid#131 -
multihash multiformats/js-multihash#104 -
multibase multiformats/js-multibase#72 -
multiaddr multiformats/js-multiaddr#159 @hugomrdias
multicodec multiformats/js-multicodec#70 -
multihashing-async multiformats/js-multihashing-async#92 -
libp2p libp2p/js-libp2p#802 -
libp2p-interfaces https://github.com/libp2p/js-libp2p-interfaces -
ipfsd-ctl
ipfs #3550 @achingbrain
err-code -

Documentation

https://github.com/ipfs/aegir/blob/master/md/ts-jsdoc.md

Improvement issues

ipfs/aegir#619 (comment)

External issues to track

TypeStrong/typedoc#1248 (comment)
microsoft/TypeScript#41672

@hugomrdias hugomrdias added exp/wizard Extensive knowledge (implications, ramifications) required awesome endeavour P0 Critical: Tackled by core team ASAP topic/docs Documentation kind/enhancement A net-new feature or improvement to an existing feature labels Mar 24, 2020
@hugomrdias hugomrdias self-assigned this Mar 24, 2020
@bluelovers
Copy link
Contributor

where the branch of it? or it not yet started?

@hugomrdias
Copy link
Member Author

not started yet, i will start in the next couple of weeks

@bluelovers
Copy link
Contributor

ipfs/js-ipfs-utils#26

@dapplion
Copy link

dapplion commented May 6, 2020

Is it possible to externally contribute to the Typescript support effort?

@bluelovers
Copy link
Contributor

@dapplion https://github.com/bluelovers/ws-ipfs/tree/master/packages/ipfs-types

@hugomrdias
Copy link
Member Author

Is it possible to externally contribute to the Typescript support effort?

@dapplion sure just pick a repo in the list and start adding JSDocs types to the top level api and mention me in the PR

@dapplion
Copy link

Is it possible to externally contribute to the Typescript support effort?

@dapplion sure just pick a repo in the list and start adding JSDocs types to the top level api and mention me in the PR

Thanks! Is it on the roadmap to migrate to Typescript?

@hugomrdias
Copy link
Member Author

no

@Gozala
Copy link
Contributor

Gozala commented May 20, 2020

I've spend some time trying to add jsdoc type annotations to the https://github.com/ipfs/js-ipfs/tree/master/packages/ipfs and encountered issue that I am not sure is possible to entype.

From what I can tell IPFS.create returns Promise<IPFS> node implementing all the core APIs. But they way it's all put together is extremely dynamic in nature:

const IPFS = require('./core')

const { api } = apiManager.update({
init: Components.init({ apiManager, print, options }),
dns: Components.dns(),
isOnline: Components.isOnline({})
}, async () => { throw new NotInitializedError() }) // eslint-disable-line require-await

Where apiManager does quite a bit of voodoo with Proxies (aside note Proxies in JS are inherently slow due to dynimac dispatch that engines aren't able to optimize, at least spidermonkey isn't so it's best unless absolutely necessary I'd recommend against them)

this.api = new Proxy(this._api, {
get: (_, prop) => {
if (prop === 'then') return undefined // Not a promise!
return this._api[prop] === undefined ? this._onUndef(prop) : this._api[prop]
}
})

update (nextApi, onUndef) {
const prevApi = { ...this._api }
const prevUndef = this._onUndef
Object.keys(this._api).forEach(k => { delete this._api[k] })
Object.assign(this._api, nextApi)
if (onUndef) this._onUndef = onUndef
return { cancel: () => this.update(prevApi, prevUndef), api: this.api }
}

So as far as I can tell IPFS instance represents merge of Record<string, Record<Key, infer T>>

const { api } = apiManager.update({
init: Components.init({ apiManager, print, options }),
dns: Components.dns(),
isOnline: Components.isOnline({})
}, async () => { throw new NotInitializedError() }) // eslint-disable-line require-await

I do not believe that can be entyped because number of types that need to be inferred is not bound. That is to say if e.g. update was taking bound number of APIs it merged together it would have being doable, which is different from arbitrary number of api objects (values of the passed dict) that are merged.

For example this is how TS entypes Object.assign (which effectively infers up to 4 args and gives up if you pass more):

interface ObjectConstructor {
    /**
     * Copy the values of all of the enumerable own properties from one or more source objects to a
     * target object. Returns the target object.
     * @param target The target object to copy to.
     * @param source The source object from which to copy properties.
     */
    assign<T, U>(target: T, source: U): T & U;

    /**
     * Copy the values of all of the enumerable own properties from one or more source objects to a
     * target object. Returns the target object.
     * @param target The target object to copy to.
     * @param source1 The first source object from which to copy properties.
     * @param source2 The second source object from which to copy properties.
     */
    assign<T, U, V>(target: T, source1: U, source2: V): T & U & V;

    /**
     * Copy the values of all of the enumerable own properties from one or more source objects to a
     * target object. Returns the target object.
     * @param target The target object to copy to.
     * @param source1 The first source object from which to copy properties.
     * @param source2 The second source object from which to copy properties.
     * @param source3 The third source object from which to copy properties.
     */
    assign<T, U, V, W>(target: T, source1: U, source2: V, source3: W): T & U & V & W;

    /**
     * Copy the values of all of the enumerable own properties from one or more source objects to a
     * target object. Returns the target object.
     * @param target The target object to copy to.
     * @param sources One or more source objects from which to copy properties
     */
    assign(target: object, ...sources: any[]): any;
}

Note that unlike array passing dict is more complicated as they come with names and I don't believe can be entyped as fixed size structs without knowing names so I don't think using trick used in Object.assign would apply.

@Gozala
Copy link
Contributor

Gozala commented May 20, 2020

Turns out I was wrong and there is a way to make the typing work

image

@dapplion
Copy link

no

@hugomrdias Is there a strong reason or ROI justification in the decision to not consider Typescript over just JSDocs types?

@achingbrain
Copy link
Member

achingbrain commented May 20, 2020

Proxies in JS are inherently slow due to dynimac dispatch that engines aren't able to optimize, at least spidermonkey isn't so it's best unless absolutely necessary I'd recommend against them

FWIW pulling the proxy voodoo out is on the list: #2762 - though more because it's complicated than because it's slow.

It's important to not jump to conclusions about performance improvements and only optimise what we can prove is a bottleneck. So far the API manager/proxy piece has not shown itself to be a bottleneck.

Is there a strong reason or ROI justification in the decision to not consider Typescript over just JSDocs types?

We want the tooling support & static analysis goodies that TS enables but there's not a lot of appetite for a ground-up rewrite in another language, particularly at the expense of items in the backlog perceived to be of higher value to the project like connectivity improvements, better content resolution, feature parity with go-IPFS, etc. It also raises the bar to entry for contributions - e.g. every TS developer knows JS, but not every JS developer knows TS. We don't want to have external definitions as it's too easy for them to become out of date when non-TS developers submit PRs, so this endeavour is to have TS types in JSDoc style comments for our external APIs only.

@dapplion
Copy link

@achingbrain I agree with your point, thanks for the explanation!

@gbaranski
Copy link

gbaranski commented Dec 7, 2020

It also raises the bar to entry for contributions - e.g. every TS developer knows JS, but not every JS developer knows TS.

But how many TS developers resigned from contributing because they have to read JS without types. And also how many resigned from using the library because of missing types. I don't think that'll be a big problem for JS developers to understand what's going on in Typescript.

@Gozala
Copy link
Contributor

Gozala commented Dec 9, 2020

@gbaranski I do not believe discussion about migration to TS is going to be productive, even if doing it had no tradeoffs at all (which it does).

What we have been doing however is gradually integrating TS via JSDoc syntax and generating typedefs from it. That way we are starting to take some advantage of type checker, more and more code has types (admittedly in less convenient form) and TS projects can start getting type inference when working with IPFS.

If you feel passionate about improving experience through TS I would encourage you to help (if you can) with our ongoing entypement efforts, there is still planty of surface to cover and plenty of @ts-ignores to address. I believe that is more effective way to address most pain points in regards to TS+IPFS

@gbaranski
Copy link

@gbaranski I do not believe discussion about migration to TS is going to be productive, even if doing it had no tradeoffs at all (which it does).

What we have been doing however is gradually integrating TS via JSDoc syntax and generating typedefs from it. That way we are starting to take some advantage of type checker, more and more code has types (admittedly in less convenient form) and TS projects can start getting type inference when working with IPFS.

If you feel passionate about improving experience through TS I would encourage you to help (if you can) with our ongoing entypement efforts, there is still planty of surface to cover and plenty of @ts-ignores to address. I believe that is more effective way to address most pain points in regards to TS+IPFS

I've added my own types for my libp2p project right here. But if You want to generate typings using JSDoc, that might be better approach that mine(rewriting API docs which seem outdated). How I can contribute?

@Gozala
Copy link
Contributor

Gozala commented Dec 10, 2020

@gbaranski given that you have wrote tsdefs for libp2p this might be a good place to start libp2p/js-libp2p#830 which contains bunch of pointers to bunch of followup work that needs to be done that was chosen to not include with initial push.

If you want to take over this libp2p/js-libp2p#831 that would be a great help as well. As per ipfs/aegir#689 (comment) we would like to replace uses of stock event emitter with a typed one https://www.npmjs.com/package/typed-events.ts

If you do end up pursuing things on libp2p end, I would suggest to coordinate with @vasco-santos who is leading the entyping efforts there to make sure we don't step on each others toes.

On the js-ipfs end there is planty of todo as well, I don't have a list handy here but here are few general groups of work:

  1. Address many @ts-ignores we had to put in place to make progress. You can pick any of them and address it which will move us closer to taking more advantage of TS.
  2. There are plenty of dependencies in js-ipfs that are not yet typed. That leads to bunch of things been any that also leaks into generated typedefs. Identifying high impact ones and fixing any one of them would be a great help & probably have largest impact on the usability of js-ipfs from TS.
  3. Currently default config sets "noImplicitAny": false which is only way we make progress. Long term we want that to turn to true. That is directly relates to No 2, since there are just to many untyped deps. Not sure what the best way to make progress here, but help in this area would also be very welcome.

If you do decide to do some work on js-ipfs end (or it's deps) I would highly encourage to read this #3413 as we have decided to take slightly change our approach to how do we type annotate things.

Also don't hesitate to reach out to myself.

@dapplion
Copy link

dapplion commented Dec 16, 2020

Long term, say 1 or 2 years, will you re-consider the position of not adopting Typescript?

Major projects in the web3 space actively used by DApp developers are already (or in progress) Typescript

If this trend continues how would it affect IPFS adoption in JS projects if it remains the only one without Typescript (or really good types #3413 (comment))?

@achingbrain
Copy link
Member

Long term, say 1 or 2 years, will you re-consider the position of not adopting Typescript?

Please see the final paragraph here: #2945 (comment)

I don't think it needs to be all-or-nothing, since:

really good types

This is what we're targeting here. If you'd like to help out with this effort, @Gozala has linked to plenty of small-ish pieces that need work, pitching in would be very much appreciated.

@dapplion
Copy link

Long term, say 1 or 2 years, will you re-consider the position of not adopting Typescript?

Please see the final paragraph here: #2945 (comment)

I don't think it needs to be all-or-nothing, since:

really good types

This is what we're targeting here. If you'd like to help out with this effort, @Gozala has linked to plenty of small-ish pieces that need work, pitching in would be very much appreciated.

Is JSDocs able (or fully-featured enough) to provide the same very rich types as "Typescript types"?

I would be very happy to contribute to a Typescript effort, but developing JSDocs is not fun at all for me personally.

However, I agree with prioritizing core functionality development over types.

@hugomrdias
Copy link
Member Author

@dapplion yes it is and not all types need to be written in jsdocs and most of them aren't already, please check the description on this issue and the PRs/links referenced there for more info.

@Gozala
Copy link
Contributor

Gozala commented Dec 16, 2020

For anyone interested in helping out moving this forward, we have decided to slightly change our current "top to bottom" strategy to "bottom upwards" strategy , meaning focusing our efforts on entyping dependencies used by js-ipfs first. This was motivated by ripple effects that added types to dependencies causes.

So if you were thinking of helping out best thing is to pick one of the libraries from the list that @hugomrdias will post here (pardon my gentle nudge 😉).

@hugomrdias
Copy link
Member Author

the list has been in the OP for quite some time now

@achingbrain
Copy link
Member

This was shipped in ipfs@0.55.x and ipfs-core@0.6.x so I think we can close this now.

If there are further issues with the types, new issues can be opened and dealt with case by case.

Thanks everyone, amazing work!

@achingbrain achingbrain unpinned this issue Jun 18, 2021
SgtPooki referenced this issue in ipfs/js-kubo-rpc-client Aug 18, 2022
TypeScript support for `ipfs` and `ipfs-http-client`

Refs: #2945
Refs: #1166 

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Co-authored-by: Alex Potsides <alex@achingbrain.net>
SgtPooki referenced this issue in ipfs/js-kubo-rpc-client Aug 18, 2022
TypeScript support for `ipfs` and `ipfs-http-client`

Refs: #2945
Refs: #1166 

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Co-authored-by: Alex Potsides <alex@achingbrain.net>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awesome endeavour Epic exp/wizard Extensive knowledge (implications, ramifications) required kind/tracking A meta-issue for tracking work P0 Critical: Tackled by core team ASAP topic/docs Documentation
Projects
No open projects
Archived in project
Development

No branches or pull requests