Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add array stream decoding #42

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

Merged
gfx merged 2 commits into msgpack:master from sergeyzenchenko:master
May 30, 2019
Merged

Add array stream decoding #42

gfx merged 2 commits into msgpack:master from sergeyzenchenko:master
May 30, 2019

Conversation

Copy link
Collaborator

@sergeyzenchenko sergeyzenchenko commented May 29, 2019
edited by gfx
Loading

#41

Copy link

codecov-io commented May 29, 2019
edited
Loading

Codecov Report

Merging #42 into master will decrease coverage by 4%.
The diff coverage is 24.44%.

Impacted file tree graph

@@ Coverage Diff @@
## master #42 +/- ##
==========================================
- Coverage 95.46% 91.46% -4.01% 
==========================================
 Files 14 14 
 Lines 772 808 +36 
 Branches 163 170 +7 
==========================================
+ Hits 737 739 +2 
- Misses 15 49 +34 
 Partials 20 20
Impacted Files Coverage Δ
src/index.ts 100% <100%> (ø) ⬆️
src/decodeAsync.ts 66.66% <20%> (-33.34%) ⬇️
src/Decoder.ts 88.96% <23.07%> (-9.6%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d22f98...50011dd. Read the comment docs.

Copy link
Member

gfx commented May 30, 2019

Looks great!

BTW Can it handle unlimited data stream? For example, something like push-notification via WebSocket that emits an item for each server-side event. GraphQL subscriptions are exactly the case.

Copy link
Member

gfx commented May 30, 2019
edited
Loading

Here is a demo for streaming deserialization of Ruby's msgpack gem which is designed and developed by the MessagePack original author:

require "msgpack"
stream = StringIO.new([
 # each item is an independent, complete MessagePack item
 MessagePack.pack(true),
 MessagePack.pack(42),
 MessagePack.pack("foo"),
 MessagePack.pack("bar"),
].join(""))
u = MessagePack::Unpacker.new(stream)
u.each do |obj|
 p obj
end

It emits true, 42, "foo", "bar", respectively. Because there is no wrapper array, the MessagePack::Unpacker can handle unlimited data stream as long as data exist.

Copy link
Collaborator Author

I see, no this is one is specifically for arrays, but I can add version what will support this kind of streams. It's pretty easy to do. But let's do it in separate PR, because it's different from this one.

Copy link
Collaborator Author

Here is the version for decoding stream of objects

async *decodeStream(stream: AsyncIterable<ArrayLike<number> | Uint8Array>) {
 for await (const buffer of stream) {
 this.appendBuffer(buffer);
 try {
 while (true) {
 let result = this.decodeSync();
 yield result;
 }
 } catch (e) {
 if (!(e instanceof DataViewIndexOutOfBoundsError)) {
 throw e; // rethrow
 }
 // fallthrough
 }
 }
 }

Copy link
Member

gfx commented May 30, 2019

Good! Can you add decodeStream() in this PR?

And can you add tests to keep test coverage?

Copy link
Collaborator Author

Can I add tests in separate PR? :) I was going to start integrating this into our project today/tomorrow.
But I will add them for sure this week.

Copy link
Collaborator Author

Let's add decodeStream in separate PR too because I want to test it first. And just to keep each PR for separate functional increment.

Copy link
Collaborator Author

I will add tests for both decodeArrayStream and decodeStream during next days.

Copy link
Collaborator Author

btw, does Karma tests requires any additionally setup in this project?
I fails with:

Error: socket hang up
 at createHangUpError (_http_client.js:343:17)
 at Socket.socketOnEnd (_http_client.js:444:23)
 at Socket.emit (events.js:205:15)
 at endReadableNT (_stream_readable.js:1137:12)
 at processTicksAndRejections (internal/process/task_queues.js:84:9) {
 code: 'ECONNRESET'
}

all the time

Copy link
Collaborator Author

Nevermind. it's because of WebStrom. Works fine from terminal

gfx reacted with thumbs up emoji

Copy link
Member

gfx commented May 30, 2019

OK! So you can add tests and others in separate PRs. Will merge it and release a new version!

Copy link
Member

gfx commented May 30, 2019
edited
Loading

BTW I use vscode to edit the code. Make sure npm run lint:fix to keep the code tidy!

@gfx gfx merged commit 0e3f706 into msgpack:master May 30, 2019
Copy link
Collaborator Author

Great! Wait for new version please. I need one more small change. I need to export Decoder and Encoder from index.ts. Are you good with it? I need to have access to raw decoder/encoder.
In our project we have custom package format that consist of

  • number
  • number
  • number
  • array

So I can't parse it using simple decode function. But it can be done by using Decoder directly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /