Skip to content
This repository has been archived by the owner on Dec 14, 2023. It is now read-only.

feat: add kind arg to encode/decode interface #75

Merged
merged 2 commits into from
Aug 1, 2019

Conversation

abelino
Copy link
Contributor

@abelino abelino commented Jul 31, 2019

In the case of using specialized schema validation/encoding/decoding we
would like to have some context on the data being passed to
serialization functions which would allow us to make a judgement on
which specialized schema [validator|decoder|encoder] to use.

This is a non-breaking change.

In the case of using specialized schema validation/encoding/decoding we
would like to have some context on the data being passed to
serialization functions which would allow us to make a judgement on
which specialized schema [validator|decoder|encoder] to use.

This is a non-breaking change.
Copy link
Owner

@denis-sokolov denis-sokolov left a comment

Choose a reason for hiding this comment

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

I’m not fully sure why you benefit from this, but I approve the general idea. I do want to ask you to change the syntax a bit, but it should have no impact on the functionality.

@@ -1,6 +1,6 @@
export type Serialization = {
encode: (value: unknown) => string;
decode: (string: string) => unknown;
encode: (value: unknown, kind: any) => string;
Copy link
Owner

Choose a reason for hiding this comment

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

Could we future-proof the syntax? Let’s make the second parameter an object for possibilities to expand it in the future, and let’s include the id while we’re at it for symmetry.

export type Serialization = {
  encode: (value: unknown, details: { kind: string, id: string }) => string;
  decode: (string: string, details: { kind: string, id: string }) => unknown;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ensure future extensibility by making the second arg an object
@abelino abelino force-pushed the feature/add-kind-arg-to-serde-fns branch from 1f479de to 979821f Compare August 1, 2019 15:25
@abelino abelino marked this pull request as ready for review August 1, 2019 15:26
@@ -100,7 +107,7 @@ export function makeWsProtocolAdapter(
id,
pushId,
lastSeenRevision,
value: serialization.encode(value)
value: serialization.encode(value, { id, kind: String(kind) })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@denis-sokolov is this correct? I haven't had time to dive too much into some of the type definitions but the type checker mentioned kind could be a few other type other than a string. Is converting to a string here ok?

Copy link
Owner

Choose a reason for hiding this comment

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

kind variable comes from the definition of our type, so its keyof Domain.

https://github.com/citrusbyte/oncilla/blob/d649bd2341a047ef71aaf6415c78048c9e68cff1/src/network/types.ts#L18-L19

Technically we could type it more strictly and precisely, but we’re doing very dynamic things and as long as our API is clean type-wise, it’s not a big deal. You’ll find a few as any in this file, too.

Let’s leave it like this for now and maybe later we’ll tighten up the types.

@abelino abelino merged commit 850a578 into master Aug 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the feature/add-kind-arg-to-serde-fns branch August 1, 2019 19:57
@abelino
Copy link
Contributor Author

abelino commented Aug 1, 2019

thanks @denis-sokolov for the review

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants