Skip to content

feat: formally expose MultihashHasher#encode#165

Closed
rvagg wants to merge 1 commit intomasterfrom
rvagg/mhencode
Closed

feat: formally expose MultihashHasher#encode#165
rvagg wants to merge 1 commit intomasterfrom
rvagg/mhencode

Conversation

@rvagg
Copy link
Member

@rvagg rvagg commented Feb 4, 2022

Continuing from multiformats/js-murmur3#19

It's always been there, and gets defined by the class, just not in the interface. SyncMultihashHasher hasn't had it, but in practice they all do have it (unless someone else has authored one since it was introduced that we don't know about).

The addition to SyncMultihashHasher is technically breaking since it adds an additional burden to the API for implementers. But it's both very new, and the implementations (we're aware of) already implement with encode(). So I'd like to argue that this go in as a semver-minor so we don't have to hassle the ecosystem with a v10 jump for something that is very unlikely to break anyone. One could also make the case that this is a fix for the introduction of SyncMultihashHasher which went out without the encode that the MultihashHasher class always had and generated in the types.

Thoughts on semverism?

It's always been there, and gets defined by the class, just not in the
interface. SyncMultihashHasher hasn't had it, but in practice they all
do have it (unless someone else has authored one since it was introduced
that we don't know about).

The addition to SyncMultihashHasher is technically breaking since it adds an
additional burden to the API for implementers. But it's both very new, and the
implementations (we're aware of) already implement with `encode()`.
* @param {Uint8Array} input
*/
digest(input: Uint8Array): Promise<MultihashDigest> | MultihashDigest
encode(input: Uint8Array): Await<Uint8Array>
Copy link
Contributor

@Gozala Gozala Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be a mistake as suddenly passing func({ digest }) will no longer satisfy requirements of the function even if it does not care about encode.

Proper way to address underlying problem is not to constraint interface further by increasing it’s surface, but rather to make our type annotations more precise. E.g it is ok to say that our multihashers implement both MultihashHasher interface along with some Encoder interface.

We can even define new interfaces here that extend both to reference it across the board. We really should avoid bloating the interface further so that it’s easier to be a valid implementation of. As shown in example it can actually be a TS breaking change, as some code may become invalid.

P.S. I’m kind of regretting name field which in most code is unecessary contstraint.

@rvagg
Copy link
Member Author

rvagg commented Apr 4, 2022

@Gozala if we do a semver-major for #161, could we slip in something here to address this problem too? Or do you think we can solve this by just introducing a new type and do it as a feat:?

@Gozala
Copy link
Contributor

Gozala commented Apr 9, 2022

@Gozala if we do a semver-major for #161, could we slip in something here to address this problem too?

Not sure I understand what you mean.

Or do you think we can solve this by just introducing a new type and do it as a feat:?

I think we really should just add a new type, for reasons described in my comment. Generally you want smallest types on params and larger ones on returns.

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 this pull request may close these issues.

2 participants