Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to access .or prop of decoders #141

Open
achingbrain opened this issue Dec 13, 2021 · 7 comments · May be fixed by #143
Open

How to access .or prop of decoders #141

achingbrain opened this issue Dec 13, 2021 · 7 comments · May be fixed by #143
Assignees

Comments

@achingbrain
Copy link
Member

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
Copy link
Contributor

Gozala commented Dec 14, 2021

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

/**
* @template {string} L
* @template {string} R
* @param {UnibaseDecoder<L>|CombobaseDecoder<L>} left
* @param {UnibaseDecoder<R>|CombobaseDecoder<R>} right
* @returns {ComposedDecoder<L|R>}
*/
export const or = (left, right) => new ComposedDecoder(/** @type {Decoders<L|R>} */({
...(left.decoders || { [/** @type UnibaseDecoder<L> */(left).prefix]: left }),
...(right.decoders || { [/** @type UnibaseDecoder<R> */(right).prefix]: right })
}))

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

* @implements {UnibaseDecoder<Prefix>}
* @implements {BaseDecoder}
*/
class Decoder {

this.decoder = new Decoder(name, prefix, baseDecode)

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
Copy link
Contributor

Gozala commented Dec 14, 2021

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

@achingbrain
Copy link
Member Author

achingbrain commented Dec 15, 2021

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 Gozala linked a pull request Dec 15, 2021 that will close this issue
@Gozala
Copy link
Contributor

Gozala commented Dec 15, 2021

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
Copy link
Contributor

Gozala commented Dec 15, 2021

@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
Copy link
Contributor

Gozala commented Dec 15, 2021

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

@Gozala Gozala closed this as completed Dec 15, 2021
@Gozala Gozala reopened this Dec 15, 2021
@achingbrain
Copy link
Member Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants