-
Notifications
You must be signed in to change notification settings - Fork 54
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
Conversation
src/codecs/codec.js
Outdated
* @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> }} |
There was a problem hiding this comment.
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.
3d8c087
to
a825c0a
Compare
In 2ea0393 I've added a roll-up interface, I've also added dist/ in here temporarily so I can consume this via gitpkg from dag-cbor to try it out. |
a825c0a
to
e5a3bda
Compare
@rvagg what is the next step here? what is the plan around removing the need for |
@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. |
@Gozala re what codecs should export - I agree that it's worth exploring an alternative that allows for encoding into existing 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 What I'd like to do is roll all the way back to just export It's still reusable as either a |
61a72af
to
d0fb6b3
Compare
See 0e9c146 for implementation of the above. I've removed 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. |
Thanks for continues effort to refine this. At this point I'm not sure there is value in having |
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. |
2c24aeb
to
d3613f3
Compare
Ready to publish, updated docs, removed the dist/ temporary assets, squashed down to a single commit, here's the message:
|
d3613f3
to
5e84e72
Compare
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
5e84e72
to
1634920
Compare
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 istherefore not very useful.
This makes
BlockCodec
just an implementer of bothBlockEncoder
andBlockDecoder
butcodec()
returns you that, plusencoder
anddecoder
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.