Skip to content

feat!: implement sync multihash hasher and export encode methods (#17) (#18)#19

Open
Gozala wants to merge 3 commits intomasterfrom
feat/sync-api-v2
Open

feat!: implement sync multihash hasher and export encode methods (#17) (#18)#19
Gozala wants to merge 3 commits intomasterfrom
feat/sync-api-v2

Conversation

@Gozala
Copy link
Contributor

@Gozala Gozala commented Jan 27, 2022

PR to release sync API as a breaking change

  • encode method in main module is gone, but still available when
    importing specic implementation e.g. '@multiformats/murmur3/murmur128'

#18)

- encode method in main module is gone, but still available when
  importing specic implementation e.g. '@multiformats/murmur3/murmur128'
@Gozala Gozala requested review from achingbrain and rvagg January 27, 2022 17:17
package.json Outdated
"./murmur32.js": {
"import": "./murmur32.js"
},
"./murmur128.js": {
Copy link
Member

Choose a reason for hiding this comment

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

Is the .js on the end intentional here? I think in most places we've dropped that.

For example: import { cid } from 'multiformats/cid', etc.

Copy link
Member

Choose a reason for hiding this comment

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

importing specic implementation e.g. '@multiformats/murmur3/murmur128'

This isn't possible with the .js on the end, it has to be import { ... } from '@multiformats/murmur3/murmur128.js'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct and yeah my comment is incorrect there. It was intentional because browsers reject imports that do not have .js at the end. So I thought this made more sense. That said we can drop those to be more inline with multiformats/cid

Copy link
Member

Choose a reason for hiding this comment

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

browsers reject imports that do not have .js at the end

So all our current code with extension-less exports are not working in browsers without bundlers? Has this just not been done before, or why haven't we heard this issue raised before since we're doing it pretty much universally now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think bundlers take care of it. Anyway this seems controversial so I'll just drop this, we can revisit along with other libs when it's more relevant.

@Gozala Gozala requested a review from achingbrain January 27, 2022 20:43
@Gozala
Copy link
Contributor Author

Gozala commented Feb 3, 2022

Updated PR to drop controversial .js extensions in exports map and added comment in ts ignored file explaining what's gonig on there.

@rvagg
Copy link
Member

rvagg commented Feb 3, 2022

OK, I'm now backtracking and getting my head around the "breaking" nature of this, and I think we need to be much clearer than we have been with things like this and avoid assumptions about TypeScript vs JavaScript usage.

In #17 we had:

With this PR import { murmur3128 } from @multiformats/murmur3 will not have a encode method, but import * murmur3128 from '@multiformats/murmur3/murmur128' will. I think this may be a right compromise.

But that wasn't true, and it's not true here. The encode is still exported on both hashers, JavaScript users encounter zero breakage.

The only way that this is breaking is the type assertion that these exports are SyncMultihashHasher; and the only reason that this being a MultihashHasher previously was OK because js-multiformats inferred that an encode was exported because of the this.encode = encode in its constructor in src/hashes/hasher.js, not from the interface defined in src/hashes/interface.ts.

So, in future let's be a bit more clear - with each other (I was confused when I tried to figure out how this encode method was gone) and with users (who aren't all using TypeScript), i.e. our commit messages and therefore changelog should state this clearly:

- encode method in main module is gone, but still available when
  importing specic implementation e.g. '@multiformats/murmur3/murmur128'

^ this just isn't true. If I run this as a .mjs then it's perfectly fine:

import { murmur3128 } from '@multiformats/murmur3'
console.log('hash', murmur3128.encode(new TextEncoder().encode('hash')))

But it breaks if I try to run it with tsc as a .ts.

But also, let's just fix up that API, I've wanted to use these hashers as non-multiformat digest generators in the past and have been stymied by this, it's just annoying and this change is making that even more obvious. Maybe we do that now and avoid a breaking change here. I'll open a PR.

@rvagg
Copy link
Member

rvagg commented Feb 4, 2022

multiformats/js-multiformats#165 - if we do this, we get to non-break here

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.

3 participants