Refactoting of decoder for better maintainability

[thephoenixofthevoid]

Should fix #64. The idea is to allow do whatever user want if user is ready to do things manually.

[thephoenixofthevoid]

I run into the same issue when using msgpack5 to get object duplex connections over binary one when implementing https://github.com/thephoenixofthevoid/remote-leveldown (it's multileveldown written from scratch using somewhat different approach to reduce complexity to zero or so), managed to pass almost all abstract-leveldown testes except for being demotivated by the only test case (https://github.com/Level/abstract-leveldown/blob/c0abe28ed5cfa1c45d5fc25585ac9c60991d48ec/test/put-get-del-test.js#L136)

[mcollina]

Can you also update the README? It’s probably better to have to two functions, one to encode undefined, and one to encode NaN.

Would they have to be functions? could they be static objects?

[thephoenixofthevoid]

Yes, having an object is reasonable but here is one thing to consider: actually we have to provide a reliable way to detect these encoded entities. Here is what I see reasonable:

interface FallbackCodec {
  encode: (data:any) => Buffer|string;
  decode: (data:Buffer|string) => any;
  type: number;
  isBuffer: boolean; // and so on
}

Here type is an positive integer from 1 to 127, a extension slot that will be taken to implement this fallback behavior.

So, if we see unsupported datatype on our way, we have to delegate encoding to encode function, check out how many bytes result takes and then prepend length and extension slot number type .

On decoding, it will undergo the same logic as if I would register:

registerDecoder(fallbackCodec.type, fallbackCodec.decode)

So we might rather implement it using this kind of API, not littering in options. I will update README too after all the rest.

[thephoenixofthevoid]

Are there only 2 cases of undefined serialization? Strictly speaking functions can be considered too. If they are implemented in somewhat simple way and do not rely on context we can send them over connection, by getting source code and passing to Function constructor on the opposite side.

It's worth noting that this MUST not be default behavior, because of security things and developer must be aware of what is going on to make use of it. Passing functions to this fallback encoder seems like the very balanced approach.

[mcollina]

So we might rather implement it using this kind of API, not littering in options.

Maybe we should just document how to do this? It comes up often as a question.

[thephoenixofthevoid]

Module throws instantly when undefined or NaN is encountered. If we simply delete these checks, bugs will be reintroduced, which were the very reason these checks exist. By the way, it seems it's the high time to split the-god-function into more manageable small ones.

[thephoenixofthevoid]

Please take a look at what kind of changes I am applying to the codebase. If you are ok with them, I will keep on that.

[mcollina]

Can you remove the yarn.lock file and add it to gitignore?

[thephoenixofthevoid]

It seems like decodingTypes is better to implement as a Map, (or object). Because there is a key existing --- no need to iterate over arrays or whatever. It might break tests, so I will do it later.

[thephoenixofthevoid]

Can you remove the yarn.lock file and add it to gitignore?

What do you think about the changes if you managed to take a look at it?

[mcollina]

Good work! you are almost done, a couple of nits.

[thephoenixofthevoid]

I should have probably stated more clearly that I am still working on it and plan to finish with encoding side today or tomorrow. I was just asking if the code transformation applied is ok, because the changes are rather deep. I will highlight once everything is done, what was changed the most. Just to be sure that the change has been noticed.

[thephoenixofthevoid]

Can you avoid committing the dist files? I'll update them before release. Yes, wouldn't it break pipeline?

[thephoenixofthevoid]

I think most of the changes to encoder.js is done.

[mcollina]

Can you please restore the dist/ files to what are on master? Thanks.

Is this ready for review? or are you planning any more work?

[thephoenixofthevoid]

Restored. I would like to have this piece of work reviewed and only then continue to work. Mostly I have finished the work I plan to do on the decoder side.

[thephoenixofthevoid]

Maybe I separate the new feature into the next PR, and roll back these changes here? The changes I introduce here are mostly refactoring to have more maintainable codebase.

[mcollina]

That’d be better, yes.

[thephoenixofthevoid]

Done that.

[thephoenixofthevoid]

Now PR is ready for a review.

[thephoenixofthevoid]

Done

[thephoenixofthevoid]

It seems travis results are misleading --- check this: https://travis-ci.org/mcollina/msgpack5/jobs/524508022

/home/travis/build/mcollina/msgpack5/test/floats.js:17
    2 ** 150,
       ^
SyntaxError: Unexpected token *

This has been in code for a while.

[mcollina]

Interesting :/ Can you take a look and fix that as well?

[thephoenixofthevoid]

It's unrelated to the changes I have made so maybe in the separate PR?

[thephoenixofthevoid]

I just wanted to point out that having check passed doesn't actually mean everything is ok. It's a bug in travis configuration. This exact issue appeared here: https://travis-ci.org/mcollina/msgpack5/jobs/381392748 11 months ago.

[thephoenixofthevoid]

Fixed, I hope.

[thephoenixofthevoid]

Oh, no https://travis-ci.org/mcollina/msgpack5/jobs/524572307 Maybe it's time to drop node v4? It's not officially maintained now anyway.

[thephoenixofthevoid]

Done. https://travis-ci.org/mcollina/msgpack5/jobs/524575211

[thephoenixofthevoid]

Waiting for this PR to be accepted because there is a next one waiting for its time: https://github.com/thephoenixofthevoid/msgpack5/blob/improve-internals/lib/encoder.js

[mcollina]

Landed

Il giorno ven 26 apr 2019 alle 06:07 thephoenixofthevoid < notifications@github.com> ha scritto:

Waiting for this PR to be accepted because there is a next one waiting for its time:

https://github.com/thephoenixofthevoid/msgpack5/blob/improve-internals/lib/encoder.js

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mcollina/msgpack5/pull/76#issuecomment-486869000, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAMXY62DEQCHTKBWNL26BLPSI2TFANCNFSM4HHTAOQA .

[kevygreen]

@mcollina This last merge has broken the build from what I can tell. It won't minify properly. Is there any plan to correct the issues with the build or should that commit be reverted until it's fixed?

[mcollina]

A PR to fix the build would be helpful. Thanks.