multiformats / js-multiformats

Multiformats interface (multihash, multicodec, multibase and CID)
Other
232 stars 54 forks source link

How to access `.or` prop of decoders #141

Open achingbrain opened 2 years ago

achingbrain commented 2 years ago

I'd like to compose a decoder, something like:

import * as b32 from 'multiformats/bases/base32'
import * as b36 from 'multiformats/bases/base36'
import * as b58 from 'multiformats/bases/base58'
import * as b64 from 'multiformats/bases/base64'
import { base32 } from 'multiformats/bases/base32'

const bases:Record<string, MultibaseCodec<any>> = {
  ...b32,
  ...b36,
  ...b58,
  ...b64
}

const baseDecoder = Object
  .keys(bases)
  .map(key => bases[key].decoder)
  .reduce(
    (acc, curr) => acc.or(curr),  // <--- fails because `.or` is not in the types though it is in the `Decoder` class
    base32.decoder
  )

It's not clear to me how I'm supposed to do this and not upset the type checker, any pointers?

I tried adding .or to the MultibaseDecoder type def but it explodes because elsewhere you need to know if you're being passed a UnibaseDecoder or a CombobaseDecoder to create a ComposedDecoder - I started trying to fix it but it got a little out of hand so I thought I'd ask instead.

Gozala commented 2 years ago

I would assume you could use static or function to compose all decoders together (so you could pass it to reduce directly)

https://github.com/multiformats/js-multiformats/blob/a4f998bdcd18088b8a37ce129ad543aa9c8aae08/src/bases/base.js#L165-L175

because b32.decoder should be compatible because it is UnibaseDecoder as per

https://github.com/multiformats/js-multiformats/blob/a4f998bdcd18088b8a37ce129ad543aa9c8aae08/src/bases/base.js#L75-L78 https://github.com/multiformats/js-multiformats/blob/a4f998bdcd18088b8a37ce129ad543aa9c8aae08/src/bases/base.js#L206

I think things do not work because of your type annotation here

const bases:Record<string, MultibaseCodec<any>> = {
  ...b32,
  ...b36,
  ...b58,
  ...b64
}

Could you just leave it out and let TS infer the type correctly ?

Gozala commented 2 years ago

I'll create pull request that adds a test with this

achingbrain commented 2 years ago

Could you just leave it out and let TS infer the type correctly ?

No, because then this errors with expression of type 'string' can't be used to index type...:

  .map(key => bases[key].decoder)

use static or function to compose all decoders together (so you could pass it to reduce directly)

Nice, I didn't spot that. Unfortunately it's defined in /src/bases/base.js which is missing from the export map so I can't import it.

If I patch that locally I can simplify a little with:

import { bases } from 'multiformats/basics'
import { or } from 'multiformats/bases/base'

const baseDecoder = Object
  .values(bases)
  .map(codec => codec.decoder)
  .reduce(or, bases.identity.decoder)

But now passing or into .reduce(or, bases.identity.decoder) errors with:

No overload matches this call.
  Overload 1 of 3, '(callbackfn: (previousValue: Decoder<string, string> | Decoder<"base58btc", "z"> | Decoder<"base58flickr", "Z"> | Decoder<"base36", "k"> | Decoder<"base36upper", "K"> | Decoder<...> | Decoder<...>, currentValue: Decoder<...> | ... 5 more ... | Decoder<...>, currentIndex: number, array: (Decoder<...> | ... 5 more ... | Decoder<...>)[]) => Decoder<...> | ... 5 more ... | Decoder<...>, initialValue: Decoder<...> | ... 5 more ... | Decoder<...>): Decoder<...> | ... 5 more ... | Decoder<...>', gave the following error.
    Argument of type '<L extends string, R extends string>(left: UnibaseDecoder<L> | CombobaseDecoder<L>, right: UnibaseDecoder<R> | CombobaseDecoder<R>) => ComposedDecoder<...>' is not assignable to parameter of type '(previousValue: Decoder<string, string> | Decoder<"base58btc", "z"> | Decoder<"base58flickr", "Z"> | Decoder<"base36", "k"> | Decoder<"base36upper", "K"> | Decoder<...> | Decoder<...>, currentValue: Decoder<...> | ... 5 more ... | Decoder<...>, currentIndex: number, array: (Decoder<...> | ... 5 more ... | Decoder<.....'.
      Type 'ComposedDecoder<string>' is not assignable to type 'Decoder<string, string> | Decoder<"base58btc", "z"> | Decoder<"base58flickr", "Z"> | Decoder<"base36", "k"> | Decoder<"base36upper", "K"> | Decoder<...> | Decoder<...>'.
        Type 'ComposedDecoder<string>' is missing the following properties from type 'Decoder<"identity", "\0">': name, prefix, baseDecode
  Overload 2 of 3, '(callbackfn: (previousValue: Decoder<"identity", "\0">, currentValue: Decoder<string, string> | Decoder<"base58btc", "z"> | Decoder<"base58flickr", "Z"> | Decoder<...> | Decoder<...> | Decoder<...> | Decoder<...>, currentIndex: number, array: (Decoder<...> | ... 5 more ... | Decoder<...>)[]) => Decoder<...>, initialValue: Decoder<...>): Decoder<...>', gave the following error.
    Argument of type '<L extends string, R extends string>(left: UnibaseDecoder<L> | CombobaseDecoder<L>, right: UnibaseDecoder<R> | CombobaseDecoder<R>) => ComposedDecoder<...>' is not assignable to parameter of type '(previousValue: Decoder<"identity", "\0">, currentValue: Decoder<string, string> | Decoder<"base58btc", "z"> | Decoder<"base58flickr", "Z"> | Decoder<...> | Decoder<...> | Decoder<...> | Decoder<...>, currentIndex: number, array: (Decoder<...> | ... 5 more ... | Decoder<...>)[]) => Decoder<...>'.
      Type 'ComposedDecoder<string>' is not assignable to type 'Decoder<"identity", "\0">'.ts(2769)

There's quite a lot going on here, could these types not be simplified a bit?

Gozala commented 2 years ago

I've investigated this further and issue is that TS fails to infer types properly, specifically it can't decide if it should treat first arg of reduce as Decoder or ComposedDecoder (which should not really matter for composition but it does for reduce).

I have WIP patch that makes it all work but I'm not happy with they way it's looking: https://github.com/multiformats/js-multiformats/pull/143/files#diff-71024965457804d5784a2cf5ed646c44ad0f02181febc252e2adc3a48711e517R188-R191

  const bases = {
      ...b32,
      ...b36,
      ...b58,
      ...b64
    }

    const composite = Object
      .values(bases)
      .map(codec => codec.decoder.composed)
      .reduce(b.or)

With above code things work as expected, but .composed field is just silly getter that returns this just so that TS can infer type properly.

Gozala commented 2 years ago

@achingbrain is manually composing with .or is too inconvenient ? I know it would be more lines of code, but things would compose as they should without having to resort to things like in the linked PR. We could further improve things by making b32, b64 module expose or composed codecs to reduce amount of or in necessary.

For the context, thinking here was that likely users would use handful of codecs that they can compose together as opposed to large amount.

Alternatively we could just expose a function like:

export const fromTable = (decoders) => new CompositeDecoder(decoders)

Which would then just accept your bases as is.

Gozala commented 2 years ago

No, because then this errors with expression of type 'string' can't be used to index type...:

BTW when you use Object.keys and then access properties by those keys TS is unable to infer the types property (because it can be any key rather than specific one for which it knows corresponding value type). Use of Object.values doesn’t have that problem (TS can just treat values as unions of all property types). And when you need both key and values then Object.entries because TS can know exact key / value types without having to treat value as union of all value types

achingbrain commented 2 years ago

is manually composing with .or is too inconvenient ? I know it would be more lines of code, but things would compose

This seems a bit backwards. The .or function exists on the Decoder class, and is useful, it's just not in the types.

I can do:

import { bases } from 'multiformats/basics'

const baseDecoder = Object
  .values(bases)
  .map(codec => codec.decoder)
  // @ts-expect-error `.or` is missing from the types
  .reduce((acc, curr) => acc.or(curr), bases.identity.decoder)

and everything works though obviously the type checker tells me it doesn't. Perhaps it could just be added to the MultibaseDecoder interface so the types reflect reality and everything is easy to reason about?