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

fix: more generic BlockCodec, remove encoder & decoder props #75

Merged
merged 1 commit into from
Apr 26, 2021

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Apr 8, 2021

This is what we probably should have done to solve the conflict in cea9063
because we ended up with a BlockCodec that has too many properties and is
therefore not very useful.

This makes BlockCodec just an implementer of both BlockEncoder and
BlockDecoder but codec() returns you that, plus encoder and decoder
properties so you have all the things and can consume them however you like.

Ref: ipld/js-dag-cbor#18 (comment)

I'm calling this a fix for a semver-patch. Do you think that's reasonable? I wouldn't mind avoiding a semver-major if we can.

* @param {Object} options
* @param {Name} options.name
* @param {Code} options.code
* @param {(data:T) => Uint8Array} options.encode
* @param {(bytes:Uint8Array) => T} options.decode
* @returns {import('./interface').BlockCodec<Code, T>}
* @returns {BlockCodec<Code, T> & { encoder: BlockEncoder<Code, T>, decoder: BlockDecoder<Code, T> }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of wish this type had a name, but naming is hard and I think we run out of names here, so unless you have a good one this is good.

@rvagg rvagg force-pushed the rvagg/moar-generic-blockcodec branch from 3d8c087 to a825c0a Compare April 10, 2021 01:58
@rvagg
Copy link
Member Author

rvagg commented Apr 10, 2021

In 2ea0393 I've added a roll-up interface, CodecFeature that combines the 3 things into a bundle. This is what we expect codec implementations to export. I've added explicit typing to json.js and raw.js (and in the process figured out why we had the weird const raw = (b) => coerce(b) thing in there that we removed recently!).

I've also added dist/ in here temporarily so I can consume this via gitpkg from dag-cbor to try it out.

@lidel
Copy link
Member

lidel commented Apr 19, 2021

@rvagg what is the next step here? what is the plan around removing the need for dist/ in repo?

@vmx
Copy link
Member

vmx commented Apr 19, 2021

@lidel the dist folder hack is only needed for testing with other depdencies. It is only needed as long as it isn't in a released version. => The dist hack is removed prior to the merge and things will work as expected.

@rvagg
Copy link
Member Author

rvagg commented Apr 21, 2021

@Gozala re what codecs should export - I agree that it's worth exploring an alternative that allows for encoding into existing Uint8Arrays, but the jump to get there is going to be large enough (and I'm still defaulting to doubtful that the underlying complexity will be worth it) that I want to get this one over the line as is and do further iteration later.

So what we have right now is this:

export interface BlockEncoder<Code extends number, T> {
  name: string
  code: Code
  encode(data: T): ByteView<T>
}
export interface BlockDecoder<Code extends number, T> {
  code: Code
  decode(bytes: ByteView<T>): T
}
export interface BlockCodec<Code extends number, T> extends BlockEncoder<Code, T>, BlockDecoder<Code, T> {}

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

So a CodecFeature ends up looking like { name:string, code:Code, encoder:BlockEncoder, decoder:BlockDecoder, encode(data:T):ByteView<T>, decode(bytes:ByteView<T>):T } and frankly the more I mull it over the more I hate it.

What I'd like to do is roll all the way back to just export BlockCodec in the codecs, like we were originally (without naming the types in those codecs explicitly): { name:string, code:Code, encode(data:T):ByteView<T>, decode(bytes:ByteView<T>):T }.

It's still reusable as either a BlockEncoder or a BlockDecoder separately. But this API is cleaner when viewed from both JS & TS, but particularly JS where { name, code, encoder: { name, code, encode }, decoder: { code, decode }, encode, decode } is just whacky.

@rvagg rvagg force-pushed the rvagg/moar-generic-blockcodec branch from 61a72af to d0fb6b3 Compare April 21, 2021 09:55
@rvagg
Copy link
Member Author

rvagg commented Apr 21, 2021

See 0e9c146 for implementation of the above.

I've removed codec() and instead just used types to assert that the exports are the right shape. But, I've added static Encoder.fromCodec(codec:BlockCodec):BlockDecoder and static Decoder.fromCodec(codec:BlockCodec):BlockDecoder instead as ways to instantiate independent BlockEncoder and BlockDecoders.

This simplifies the burden on codecs and I think opens the door for more novel behavior here .. maybe. This API smells less to me at least. @Gozala I'd love your take on this.

@Gozala
Copy link
Contributor

Gozala commented Apr 22, 2021

I've removed codec() and instead just used types to assert that the exports are the right shape. But, I've added static Encoder.fromCodec(codec:BlockCodec):BlockDecoder and static Decoder.fromCodec(codec:BlockCodec):BlockDecoder instead as ways to instantiate independent BlockEncoder and BlockDecoders.

Thanks for continues effort to refine this. At this point I'm not sure there is value in having Encoder and Decoder classes. I think it might be better to just remove them, we can always reintroduce them if we end up adding some methods on them.

@rvagg
Copy link
Member Author

rvagg commented Apr 22, 2021

At this point I'm not sure there is value in having Encoder and Decoder classes

Removed, we can revisit if/when we have a chance to rethink the codec composability ideas back in here. For now we have a much simpler codec interface and some clean types.

I'm going to go back and redo the codecs now to conform to this.

@rvagg
Copy link
Member Author

rvagg commented Apr 26, 2021

Ready to publish, updated docs, removed the dist/ temporary assets, squashed down to a single commit, here's the message:

fix!: simplify the codec interface, remove helper methods & classes …

BREAKING CHANGE:

* Removes the codecs/codec export - there is no longer a helper function to
  building codecs, they should instead assert that they conform to the
	`BlockCodec` type, see json and raw examples. The `codec()` helper and
	`Encoder` and `Decoder` classes have been removed. Use type assertions to
	conform to simple signatures for now. In the future we may introduce more
	composable codec functionality which may require codecs to lean on additional
	helpers and classes.
* Fix bases/base64 type export to be usable

@rvagg rvagg force-pushed the rvagg/moar-generic-blockcodec branch from d3613f3 to 5e84e72 Compare April 26, 2021 03:27
BREAKING CHANGE:

* Removes the codecs/codec export - there is no longer a helper function to
  building codecs, they should instead assert that they conform to the
	`BlockCodec` type, see json and raw examples. The `codec()` helper and
	`Encoder` and `Decoder` classes have been removed. Use type assertions to
	conform to simple signatures for now. In the future we may introduce more
	composable codec functionality which may require codecs to lean on additional
	helpers and classes.
* Fix bases/base64 type export to be usable
@rvagg rvagg force-pushed the rvagg/moar-generic-blockcodec branch from 5e84e72 to 1634920 Compare April 26, 2021 03:28
@rvagg rvagg merged commit 076290c into master Apr 26, 2021
@rvagg rvagg deleted the rvagg/moar-generic-blockcodec branch April 26, 2021 03:31
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.

4 participants