Discussion: Improving typings

[Gozala]

Constraints

Here is the set of constraints following proposal attemts to satisfy (please call
out if you find something is missing or doesn't belong)

• Provide a way to ensure that ipfs-core and ipfs-http-client implement
  same base API.
  • They could intentionally diverge a bit (e.g. http-client takes extra HTTPOptions)
    and so could ipfs-core but there should be common subset between two.
    • Compatibility should be ensured type checker as opposed to discipline or
      peer review
• Should require no type annotation repetitions.
• Types should be simple and stright forward, meaning avoid
  magic types
  that are hard to understand.
• Cross package type dependecies should not depend on internal file structure
  (as much as possible)

Proposal

I propose to satisify all of the listed constraints by doing following

1. Create a designated directory for reusable type definitions (e.g.
   src/interface or whatever wins bikeshed contest and have component specific
   interface definitions for ipfs.pin, ipfs.files, ipfs.dag, etc..

   Note: This still creates file structure but it is agnostic of implementation
   details. Alternative could be to use TS namespaces,
   but they are left over from pre-esm days in TS and seems like non futureproof
   approach, which is why using naming convetion seems like a better option
   ipfs.pin -> interface/pin, ipfs.files -> interface/files, etc...

   Below is illustration of what interface/pin.ts could look like:

   import CID from "cids"
   import { AbortOptions } from "./utils"
   
   export interface API {
     addAll (source: ToPins, options?: AddOptions & AbortOptions): AsyncIterable<CID>
     add(source: ToPin, options?: AddOptions & AbortOptions): Promise<CID>
     rmAll(source: ToPins, options?: RemoveOptions & AbortOptions): AsyncIterable<CID>
     rm(source: ToPin, options?: RemoveOptions & AbortOptions): Promise<CID>
     ls(options?: ListOptions & AbortOptions): AsyncIterable<PinEntry>
   }
   
   export type ToPin =
     | CID
     | string
     | ToPinWithPath
     | ToPinWithCID
   
   export type ToPinWithPath = {
     path: string
     recursive?: boolean
     metadata?: any
     cid?: undefined
   }
   
   export type ToPinWithCID = {
     cid?: CID
     recursive?: boolean
     metadata?: any
     path?: string
   }
   
   export type ToPins =
     | ToPin
     | Iterable<ToPin>
     | AsyncIterable<ToPin>
   
   export type Pin = {
     path: string | CID
     recursive: boolean
     metadata?: any
   }
   
   export type PinEntry = {
   cid: CID
   type: PinType
   }
   
   export type PinType =
     | 'direct'
     | 'recursive'
     | 'indirect'
   
   export type RemoveOptions = {
     /**
      * Recursively unpin the object linked
      */
     recursive?: boolean
   }
   
   export type AddOptions = {
     lock?: boolean
   }
   
   type ListOptions = {
     /**
      * CIDs or IPFS paths to search for in the pinset
      */
     paths?: Array<string|CID>
     /**
      * Filter by this type of pin ("recursive", "direct" or "indirect")
      */
     type?: PinType | 'all'
   }

2. Each IPFS component that can provide @implements {API} annotation that type
   checker will ensure.

   Note: That does imply that implementer implements that exact API or it's
   superset which enables deriviation across ipfs-core and
   ipfs-http-client like added options in the later.

   Below is illustration of how ipfs-core can use this on the same pin
   component:

   /**
    * @implements {API}
    */
   class PinAPI {
     /**
      * @param {Object} config
      * @param {GCLock} config.gcLock
      * @param {DagReader} config.dagReader
      * @param {PinManager} config.pinManager
      */
     constructor ({ gcLock, dagReader, pinManager }) {
       // ...
     }
   }
   
   /**
    * @typedef {import('../../interface/pin').API} API
    */

   This will work regardless if we choose to do Incidental Complexity: Dependency injection is getting in the way #3391 or a slight variation on
   keep the current style used everywhere in chore: make IPFS API static (remove api-manager) #3365. If we keep
   the current style above will look as:

   const createAddAll = require('./add-all')
   
   /**
    * @implements {API}
    */
   class PinAPI {
     /**
      * @param {Object} config
      * @param {GCLock} config.gcLock
      * @param {DagReader} config.dagReader
      * @param {PinManager} config.pinManager
      */
     constructor ({ gcLock, dagReader, pinManager }) {
       this.addAll = createAddAll({ gcLock, dagReader, pinManager })
       // ...
     }
   }
   
   /**
    * @typedef {import('../../interface/pin').API} API
    */

   Where ./add-all.js looks like:

   /**
     * @param {Object} config
     * @param {GCLock} config.gcLock
     * @param {DagReader} config.dagReader
     * @param {PinManager} config.pinManager
     */
   module.exports = ({ pinManager, gcLock, dagReader }) => {
     /**
       * Adds multiple IPFS objects to the pinset and also stores it to the IPFS
       * repo. pinset is the set of hashes currently pinned (not gc'able)
       *
       * @param {ToPins} source - One or more CIDs or IPFS Paths to pin in your repo
       * @param {AddOptions & AbortOptions} [options]
       * @returns {AsyncIterable<CID>} - CIDs that were pinned.
       */
     async function * addAll (source, options = {}) {
       // ...
     }
   }
   
   /**
    * @typedef {import('../../interface/pin').API} API
    * @typedef {import('../../interface/pin').ToPins} ToPins
    * @typedef {import('../../interface/pin').AddOptions} AddOptions
    * @typedef {import('../../interface/util').AbortOptions} AbortOptions
    * ...
    */

   Note that all the types are imported from interface/*.

   And if we choose to reduce to remove dependency incejection (as per Incidental Complexity: Dependency injection is getting in the way #3391) things work just as well (better actually)

   /**
    * @implements {API}
    */
   class PinAPI {
     /**
      * @param {Object} config
      * @param {GCLock} config.gcLock
      * @param {DagReader} config.dagReader
      * @param {PinManager} config.pinManager
      */
     constructor ({ gcLock, dagReader, pinManager }) {
       // ...
     }
     /**
       * Adds multiple IPFS objects to the pinset and also stores it to the IPFS
       * repo. pinset is the set of hashes currently pinned (not gc'able)
       *
       * @param {ToPins} source - One or more CIDs or IPFS Paths to pin in your repo
       * @param {AddOptions & AbortOptions} [options]
       * @returns {AsyncIterable<CID>} - CIDs that were pinned.
       */
     addAll(source, options) {
       return addAll(this.state, source, options)
     }
     // ...
   }

3. ipfs-http-client will do more or less the same, with the
   difference that config is whatever it is in ipfs-http-client:

   /**
    * @implements {API}
    */
   class PinAPI {
     /**
      * @param {Object} config
      * ....
      */
     constructor (config) {
       // ...
     }
     // ...
   }
   
   /**
    * @typedef {import('ipfs-core/src/interface/pin').API} API
    */

   Here as well we can continue with our current approach of
   dependency injections:

   const createAddAll = require('./add-all')
   
   /**
    * @implements {API}
    */
   class PinAPI {
     /**
      * @param {Object} config
      * ....
      */
     constructor (config) {
       this.addAll = createAddAll(config)
       // ...
     }
     // ...
   }
   
   /**
    * @typedef {import('ipfs-core/src/interface/pin').API} API
    */

   Or move to dependency injection free approach:

   const addAll = require('./add-all')
   
   /**
    * @implements {API}
    */
   class PinAPI {
     /**
      * @param {Object} config
      * ....
      */
     constructor (config) {
       // ...
     }
     /**
      * @param {ToPins} source
      * @param {AddOptions & ClientOptions} [options]
      */
     addAll (source, options) {
       return addAll(this.state, source, options)
     }
     // ...
   }
   
   /**
    * @typedef {import('ipfs-core/src/interface/pin').API} API
    * @typedef {import(ipfs-core/src/interface/pin').ToPins} ToPins
    * @typedef {import('ipfs-core/src/interface/pin').AddOptions} AddOptions
    * @typedef {import('../../util').HTTOptions} ClientOptions
    */

   Note: That implementation uses ClientOptions in place of AbortOptions (which it extends) while the class itself also implements pin API that type checker will ensure.

Additional notes

• I believe @hugomrdias mentioned that in the future release of TS @typedef {import('ipfs-core/src/interface/pin')} Pin would gain namespace like behavior which will reduce amount of imports in the examples.
• I expect that future improvements of TS to allow following patterns as well:

   import * from "./pin"
   export pin
   // ...

  Which will basically provide namespace like behavior so we could just have a single import.

[achingbrain]

Sounds good. Moving them somewhere central instead of having everything depend on ipfs-core also seems sensible.

🚲 🏡 Put them in the interface-ipfs-core module as that's what defines the core-api, which is what we're trying to type. 🚲 🏡

[Gozala]

I'll follow this pattern in implementing #3293

[Gozala]

@achingbrain do you have

🚲 🏡 Put them in the interface-ipfs-core module as that's what defines the core-api, which is what we're trying to type. 🚲 🏡

Where in interface-ipfs-core do you think it should go ? Currently ./src/ follows same layout as components but hosts tests. So we should either have parallel tree e.g. ./type or naming convention to diff between test modules and interface defs. For the #3293 I'll just go with ./type/ happy restructure as necessary.

[achingbrain]

I don't have a strong opinion TBH, ./type is fine unless there's a convention we can follow?

[hugomrdias]

i would prefer types but its just a nit, we are already using types.ts in other repos so for a big group of types a types folder would make sense.

[Gozala]

@hugomrdias @achingbrain I would like to discuss / reach a consensus in regards to function overloads that have certain issues. This change e1ec19d is an illustrative of the genera problem so I'll use that as an example.

We have certain APIs that produce different output based on the input (in terms of types and shapes). Let's consider this example from ipfs.pin.remote.ls that has following interface (it is simplified here to keep discussion on point):

const example = async (api:LS) => {
  for await (const entry of api.ls()) {
    entry.stat.pinned
    //    ^^^^ Property 'stat' does not exist on type 'Entry'
  }

  for await (const entry of api.ls({ stat: true })) {
    entry.stat.pinned
    // works fine because stat was passed
  }
}

interface LS {
  ls(options?:AbortOptions): AsyncIterable<Entry>
  ls(options:{stat:true} & AbortOptions): AsyncIterable<EntryWithStat>
}

interface AbortOptions { signal?: AbortSignal }
interface Entry { /** ... not important */}
interface EntryWithStat extends Entry { stat: Stat }
interface Stat {
  pinned: number
  /* ... not important */
}

As you can see with about interface definitions type checker can identify if stat will be in place or not and catch problems at compile time. Problem however is it is pretty much impossible to have an implementation in JS that is able to satisfy that interface, without sprinkling any types over it, which in turn can easily miss or/and introduce other issues.

Another alternative is to use less precise types, here is an example illustrating that

const example = async (api:LS) => {
  for await (const entry of api.ls()) {
    entry.stat.pinned
    //    ^^^^ Object is possibly 'undefined'
  }

  for await (const entry of api.ls({ stat: true })) {
    entry.stat.pinned
    //    ^^^ Object is possibly 'undefined'
  }

  for await (const entry of api.ls({ stat: true })) {
    if (entry.stat) {
      entry.stat.pinned
      // works here
    } else {
      // what to do here though ?
    }
  }
}



interface LS {
  ls(options?:{stat?:true} & AbortOptions): AsyncIterable<Entry>
}

interface AbortOptions { signal?: AbortSignal }
interface Entry { stat?: Stat /** ... not important */}
interface Stat {
  pinned: number
  /* ... not important */
}

Doing that (which is mostly what we have today) makes API more cumbersome to use for others and ourselves as well. Furthermore it introduces a questions like what to do if stat is supposed to be there but is not.

Finally there is yet another approach, that is not overloading functions in first place as this example illustrates

const example = async (api:LS) => {
  for await (const entry of api.ls()) {
    entry.stat.pinned
    //    ^^^^ Property 'stat' does not exist on type 'Entry'.(2339)
  }

  for await (const entry of api.lsWithStat()) {
    entry.stat.pinned
    // Stats are there
  }
}



interface LS {
  ls(options?:AbortOptions): AsyncIterable<Entry>
  lsWithStat(options?:AbortOptions):AsyncIterable<EntryWithStat>
}

interface AbortOptions { signal?: AbortSignal }
interface Entry { /** ... not important */}
interface EntryWithStat extends Entry { stat: Stat }
interface Stat {
  pinned: number
  /* ... not important */
}

I personally believe that last approach is best compromise because:

• It's easy to type and implement
• Usage is more convenient, no unnecessary checks that property is there or questions what to do if it is not.
• It does not introduce any types which can lead to other issues.
• It forces implementation to uphold invariants (like when stat should be there and when it should not be), without which it's easier to make a mistake of not having stat when it's expected.

Tradeoff is however just like with (add & addAll) API surface becomes larger even if simpler & more convenient to use.

What do you think the right compromise here is, or is would it vary from case by case basis ? If there is one we should probably encode it into aegir ts guide.