js-multiformats icon indicating copy to clipboard operation
js-multiformats copied to clipboard

WIP: or combinator improvement

Open Gozala opened this issue 4 years ago • 6 comments

Attempt to fix #141

I like that this patch gets rid of UnibaseDecoder all together which simplifies things a bit. However I do not like silly composed getter it introduces just so that array reduce will not get confused by the fact that argument is either Decoder<B, P> or CompositeDecoder<P>.

I think it would be better to either

  1. Accept the fact that TS inference is going to be poor and compose things statically (as in manually listing all operands base32.decoder.or(base64.decoder).or(base58.decoder).... Not ideal but I'm guessing number of decoders isn't going to be really big anyway.
  2. Export another function like export const fromTable = (decoders) => new CompositeDecoder(decoders) so that one could simply provide table when needed as opposed to building up decoder with or operator. P.S.: I originally left this out, because or works with composed and single base decoders, table on the other hand requires only single base decoders.

Gozala avatar Dec 15 '21 09:12 Gozala

silly composed getter it introduces

Yeah, that is pretty silly. Although maybe the clean-up in there is worth it, it's quite nice now, aside from that.

I don't mind your fromTable idea either, I reckon that's going to cover the common use-cases of this. And we could copy the pattern if/when we get around to doing codecs because we have js-ipfs doing its own code->codec mapping in multiple places that would be nice to try and simplify with a single call through a composed stack. Your ipnft thing could probably do with it too. Mostly you just want to say "I have some encoders, squish them together so I have one thing please" and having to chain .or() is going to lead to the typical reduce() pattern to make it scale cleanly.

rvagg avatar Dec 16 '21 11:12 rvagg

"I have some encoders, squish them together so I have one thing please" and having to chain .or() is going to lead to the typical reduce() pattern to make it scale cleanly.

I don't know if it's not the long list of things doing .or(a).or(b).or(c)... isn't really that cumbersome IMO, but yeah supporting long lists in some way seems like a good idea.

Gozala avatar Dec 17 '21 22:12 Gozala

I think I'll make few more changes to this pr specifically:

  1. Get rid of .composed getter this introduces.
  2. Introduce fromTable function that take care of composing things.
  3. Introduce some function that turns Decoder<B, P> -> CompositeDecoder<P> so it could be used if needed in place of silly getter. Mostly so that fromTable could be implemented.

Gozala avatar Dec 17 '21 22:12 Gozala

@Gozala : what are the next steps here? Is this work being carried forward?

BigLep avatar Jan 28 '22 15:01 BigLep

(question from triage) @Gozala would it be less disruptive if we update the interface instead?

lidel avatar Feb 04 '22 15:02 lidel

(question from triage) @Gozala would it be less disruptive if we update the interface instead?

I’m not sure how would you do that without going and changing an implementation to comply with it, unless you ts-ignore it away.

@Gozala : what are the next steps here? Is this work being carried forward?

Sorry I missed that comment. We can either take this as is which is better that what we have and addresses the issue at hand, or I can try to find time to make changes discussed. Those changes could be future improvements however as they would just add more functions.

Gozala avatar Feb 17 '22 11:02 Gozala