Skip to content

Conversation

i-kovalyov
Copy link
Contributor

Fixed Stream decode processing error:
after C0 byte (nil) stream switched to "End of stream state",
added test.
Minor changes made in modules formatting to satisfy modules
formatting rules checked in tests.

after C0 byte (nil) stream switched to "End of stream state",
added test.
Minor changes made in modules formatting to satisfy modules
formatting rules checked in tests.
@i-kovalyov i-kovalyov mentioned this pull request Apr 4, 2018
Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I think the proper fix is to add a new option to the streams ‘wrap: true’, that will wrap any incoming and outgoing message in an object { value }. In this way, null could be correctly decoded.

lib/streams.js Outdated
}

Base.prototype.emit.apply(this, arguments)
}
Copy link
Owner

Choose a reason for hiding this comment

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

This is a very bad practice, and it will only work for on(‘data’) handlers and not piped streams.

…ility reasons.

When wrap is true decoded items are passed in form of {value: data} instead of data.
Using wrap=true for decoder stream allows decoding of C0 byte (nil).
test/streams.js Outdated

var pack = msgpack()
var decoder = pack.decoder()
decoder.wrap = true
Copy link
Owner

Choose a reason for hiding this comment

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

can you pass this as an option instead?

@mcollina
Copy link
Owner

mcollina commented Apr 5, 2018

Can you also add the same for the encoder?

@i-kovalyov
Copy link
Contributor Author

Sorry, to confirm for encoder.
Do you want to add "unwrap" option for encoder?
Using unwrap = true encoder can
encode obj.value instead of obj.

Using this option we can pass decoder output to encoder.

@mcollina
Copy link
Owner

mcollina commented Apr 6, 2018

The overall goal is to have source.pipe(encoder).pipe(decoder).pipe(sink) have the same input/output sata. I would keep the name of the option the same, but do what make sense to you.

@i-kovalyov
Copy link
Contributor Author

Sorry, don't understand how to get same input and output data in this scanario:
source.pipe(encoder).pipe(decoder({wrap:true})).pipe(sink).

For example when I pass null to encoder it will translate it to C0 byte, then
decoder will decode C0 byte to {value:null}.
I can't translate output to null using extra transformation at encoder level.

Can you explain please?

@mcollina
Copy link
Owner

mcollina commented Apr 6, 2018

What I mean is that the encode takes objects in the form of { value }.

When wrap = true decoder stream outputs {value:data} instead of data,
encoder stream encodes data extracted from {value:data}.
Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Can you add a test for the encoder as well? Also something that exercise both at the same time in a pipeline.

Can you also add the doc for this feature?

Good work!

encoder/decoder streams chain with wrap options.

Wrap option for encoder/decoder streams documented in readme.md
lib/streams.js Outdated
}

Base.call(this, opts)
this._unwrap = ('wrap' in opts) && opts.wrap
Copy link
Owner

Choose a reason for hiding this comment

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

if the option is wrap, the property should be ._wrap.

README.md Outdated
Builds a stream in object mode that encodes msgpack.

Supported options:
wrap.
Copy link
Owner

Choose a reason for hiding this comment

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

can you make this a list:

Supported options:

* `wrap`, put the text inline.

README.md Outdated
Builds a stream in object mode that decodes msgpack.

Supported options:
wrap.
Copy link
Owner

Choose a reason for hiding this comment

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

same as the encoder.

Minor changes in documentation for wrap option of encoder/decoder.
@mcollina
Copy link
Owner

Can you please rebase? This now conflicts with master.

Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

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.

2 participants