Named exports

[vmx]

This PR replaces all default exports with named exports. This change was triggered by using this module with CommonJ together with TypeScript.

As it does change the exports, I went all in an uppercased all the exports that feel more like objects. When I was using this library I wonder why those are all lowercase. I don't know if people agree on that. I'm happy to take those changes back. Please do bikeshed on those names, I'd really like to get this right, so that we don't need to change that in the future again. Let's do one breaking change now.

Also please do take the time to review, I'd like to get this one done rather soon as I currently work on integrating js-multiformats into js-ipfs and the sooner this is merged the less pain causes it for me.

[vmx]

This PR is related to #66 and mikeal/ipjs#11.

[mikeal]

Can we only upper case the exports that are classes that can be instantiated with “new.” That’s a code convention I’m not willing to part with because differentiating these is really important in pure JS.

[vmx]

@mikeal Oh I see. I think I usually uppercase "class-like" objects, I haven't thought about only uppercasing only ones you can instantiate with new. I'll change it.

[vmx]

One more thing. I was using const CID = require('multiformats/cid) and it broke in some browser bundling at runtime. I haven't looked deeper what the actual issue was, it might be completely unrelated, but when I used const { CID } = require('multiformats') it worked. So this might be another data point on using names exports might make sense.

[mikeal]

ugh. it’s really painful that the state of the JS package ecosystem is effectively “can’t use default exports, sorry.”

exporting one named import just feels so wrong, but i guess it’s what we have to do.

[vmx]

Sorry for another force-push. The only change is in the README, I changed one import that shouldn't have been changed.

I found this while I double-checked with my current js-unixfs work if things pan out properly. Good news is, with this change I don't get any unexpected TypeScript typechecker errors.

[Gozala]

Looks good to me, suggested few changes, but I also don’t feel very strong about them

[vmx]

The reason I called them rawCodec and jsonCodec is that I would expect people rather not rename thing (it's simpler) and json and raw is just so generic. Though I also don't have a strong opinion. What do others think?

[Gozala]

The reason I called them rawCodec and jsonCodec is that I would expect people rather not rename thing (it's simpler) and json and raw is just so generic. Though I also don't have a strong opinion. What do others think?

this makes me wonder if we should just ditch the class wrappers (which I’m one to be blamed for intruding) and instead just do something like:

export const { encode, decode, encoder, decoder, code, name } = codec({ name: ‘json’, ... })

that way users choose the name they want by doing import * as whatever from “multiformats/codec/json”

I think this is also what @mikeal was advocating for in the past.

[mikeal]

raw and json are preferred for sure.

[vmx]

OK, I changed to just raw and json, it makes the diff even smaller. I think once @rvagg approved it's ready to be merged (whoever feels like merging it).

[rvagg]

@Gozala

this makes me wonder if we should just ditch the class wrappers (which I’m one to be blamed for intruding) and instead just do something like:

export const { encode, decode, encoder, decoder, code, name } = codec({ name: ‘json’, ... })

This came up in Alan's original ipld/js-dag-cbor#14, that having the wrapper seemed to make it easier to introduce typing on the export.

I'm actually fine either way as long as we can maintain consistency. I have some additional pieces I export for js-dag-pb that are not in the standard codec() so it'd be nice to be flexible (while also catering nicely for consistent types throughout the codec ecosystem that standardises on this!): https://github.com/ipld/js-dag-pb/blob/63506baf9a1df02452ebb71d6895a537eb336cdb/index.js#L234

[rvagg]

no strong opinion other than just make it work which seems like a pipe dream with ESM

would like to see @Gozala's point resolved about what the codecs export because bundling that as a breaking change would be good before this is merged

[vmx]

Yet another force push. This removed the Codec class as @Gozala suggested. I made this change as

• @Gozala proposed it
• @rvagg wanted to have it discussed
• @mikeal seems to learn towards plain objects anyway

And overall it really seems to be a nicer way of doing it. It means less breaking changes (at least for CJS) and we can now have custom names of the imports without much renaming.

[Gozala]

Looks good to me, just want proposed type annotation to ensure that code in the returned thing is inferred properly.

[Gozala]

Can you please add this to make sure that return complies with interface def

Suggested change

	* @param {(bytes:Uint8Array) => T} options.decode
	* @returns {import('./interface'). BlockCodec<Code, T>}

[rvagg]

@Gozala note that this change forces further changes to BlockCodec because encoder and decoder are part of the return value now, so we then have to adjust the types to:

-export interface BlockCodec<Code extends number, T> extends BlockEncoder<Code, T>, BlockDecoder<Code, T> { }
+export interface BlockCodec<Code extends number, T> extends BlockEncoder<Code, T>, BlockDecoder<Code, T> {
+  encoder: BlockEncoder<Code, T>,
+  decoder: BlockDecoder<Code, T>
+}

Which I've done in my fixup!, but you may have opinions about that!

[Gozala]

Oh good catch, I forgot Codec was just Encoder + Decoder without those encoder / decoder props. Thanks!

[rvagg]

I've pushed 2 commits:

1. a fixup! that addresses @Gozala's concerns (a3553aa - also see note above about the changes this forces on BlockCodec which may be undesirable)
2. an update-deps change which also removes lodash.transform (probably exists for legacy reasons but not needed now)

[rvagg]

Latest commit deals with the problem that #66 is trying to address and a little bit more. It publishes now with a "main" which is required by some bundlers (@vmx you may run into that problem integrating into js-ipfs and running the examples) - this is a breaking change which is why I'd like to bundle it in here if possible. The "main" pointing to "cjs/src/index.js" messes up types for imports of 'multiformats' so there's a single type mapping for that in "typesVersions" which appears to fix it up.

The other thing this does is fix the *.ts.map problem where the mappings point to ../../src/... files which are not published. There's two things that I've done to fix that:

1. Copy in src/ and vendor/ into dist/ (and I'm fine with publishing those, I think it's a positive to include original source files with the package even if it increases the size very slightly)
2. Run tsc inside dist/ so everything is local.

@Gozala you may want to look at 1f91319 to sanity check. It passes here and it passes when this package is consumed by js-car, at least as an ESM. @vmx hopefully you have as much success consuming this as CJS.

If it's a +1 from both of you then it's a +1 for me. @vmx if this gets held up by my latest change for whatever reason and you're keen to get this closed out, then feel free to remove that to move forward and I'll pursue that in a separate PR.

[vmx]

I tried the current version with my js-ipfs-unixfs branch and it worked without problems. The type checking passed as expected. This is a 👍 from me (I can't approve this PR as I opened it).

@rvagg I'd be happy if you (once @Gozala approved) would merge this PR. I'm a bit nervous about all the auto release magic in this repo. Feel free to fixup/squash/force-push as much as you like.

[rvagg]

merged, but the auto-publish didn't pick up the breaking changes so it's been published as a patch! I think it's gone by the head commit, such kind of sucks. I'm going to fix it by publishing a patch revert and then publish a major with a breaking head.

also, @Gozala I had to remove the "prepare" script because the publish is done inside dist/ and the devdeps aren't available inside there to run tsc. We'll have to address that (somehow) if we want a prepare but for now the workflow has it baked in for test and publish at least.

[rvagg]

yeah, maybe a good thing you didn't merge this yourself @vmx, here's a post-mortem (mainly for @mikeal because its your publish stack that's broken here in a couple of ways):

1. merge-release in the auto-publish tooling decided that this was a patch-release in spite of the multiple obviously breaking commit messages in the list. v4.6.4.
2. I quickly reverted all of the commits, added a new empty-line commit as a chore: at the HEAD (assuming that merge-release is only looking at the HEAD because it's designed for publish-every-pull-request where they are usually a single squashed commit), in Bad publish bump #73. Merged that but it decided it was a major-release! So we have a v5.0.0 that's identical to v4.6.3.
3. I then noticed that the original v4.6.4 didn't publish after I tried to consume it as a dependency for testing js-car! The action doesn't get the non-zero exit status from the npm publish so it continues as normal, including making the tag here and giving a ✅ for the Action.
4. I replayed the list of commits here, adding back an empty-line removal commit as a feat!: at the top that copies the original feat!: commit message, in Bad publish re-bump #74. That merge properly triggered a major-release so now we have a v6.0.0 that's what we actually intended (and it works for js-car btw, so yay).