metarhia / metacom

RPC communication protocol for Metarhia stack 🔌
https://metarhia.com
MIT License
80 stars 40 forks source link

Decouple network layer and packet ser/de #421

Closed mprudnik closed 1 year ago

mprudnik commented 1 year ago
tshemsedinov commented 1 year ago

Refs: https://github.com/metarhia/Contracts/blob/master/doc/Metacom.md

mprudnik commented 1 year ago

@tshemsedinov updated the naming to match the contract docs. Few remaining questions (comments in code):

mprudnik commented 1 year ago

@tshemsedinov Storing handler functions in an object is ok if arrow functions are used since this will be bound automatically. Here's sample code with test - https://github.com/mprudnik/metacom/blob/sample-client-test/test/client.js.

mprudnik commented 1 year ago

@tshemsedinov Done most of the requested changes, the remaining questions are only MetaWritable initialization and naming. And of course CHANGELOG and d.ts.

mprudnik commented 1 year ago

@tshemsedinov all change requests resolved.

mprudnik commented 1 year ago

@tshemsedinov In general, having serializers receive data as objects is easier to extend, much more readable (since you know precisely what is what), and makes work with optional fields easier (in cases of stream, and version for call). Here it is done the same - https://github.com/metarhia/metacom/blob/master/lib/streams.js#L134. Why not pass id, name and size as positional arguments, instead of passing those as options object? So although at first sight, this seems redundant:

({ id, name, size, status }) => ({
    type: 'stream',
    id,
    name,
    size,
    status,
  })

it makes future changes easier to have.

tshemsedinov commented 1 year ago

Landed commit rolled back, it changes expected structures, I'll rewrite it later, we can't include it release as of now Work moved here: https://github.com/metarhia/metacom/pull/425