Skip to content
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

Same functionality as accum? #22

Closed
dmitrig01 opened this issue Mar 4, 2014 · 10 comments
Closed

Same functionality as accum? #22

dmitrig01 opened this issue Mar 4, 2014 · 10 comments

Comments

@dmitrig01
Copy link

What's the difference between this project and https://github.com/jeffbski/accum? Similar issue filed there at jeffbski/accum#1

@max-mapper
Copy link
Owner

looks like the same thing, probably just a case of calling the same thing two different names

@dmitrig01
Copy link
Author

If it's the same thing, would it make sense to try and merge the projects?

@jeffbski
Copy link

jeffbski commented Mar 5, 2014

They are very similar. However one of accum's original use cases was to make sure that if multibyte chars were split across packets in a stream, that they were properly concatenated back together before converting to string otherwise they can be corrupted.

Since I didn't have that use case in a test, the code regressed over time, but after realizing that today I added a test and fixed the code back so it handles this situation properly.

See multibyte char split test -jeffbski/accum@858c22c

and the fix - jeffbski/accum@33d2961

To fix concat-stream similarly it involves doing Buffer.concat before the toString('utf8') or similar rather than just converting the individual packets.

@max-mapper
Copy link
Owner

@dmitrig01 there are plenty of modules with similar functionality on NPM, IMO it isnt worth the effort to try and merge projects in this scenario. if you want to send me a PR to the concat-stream readme that mentions similar modules in an effort to avoid confusion then I would merge it.

@jeffbski at the moment concat-stream doesn't call .toString() for streams of buffers, it just does Buffer.concat so multibyte chars shouldnt get messed up

@jeffbski
Copy link

jeffbski commented Mar 5, 2014

@maxogden I may have misread the concat-stream code but I thought that if one specifies

concat({encoding: 'string'}, cb)

That this would force a conversion to string from the buffer packets by iterating over the packets and calling toString on each one individually which causes the problem if a multi byte char happen to get split over two packets in the stream.

Please ignore me if I am crazy, that's just what it looked like from my quick look at the code.

Ps. I plan to add concat-stream to my readme under related projects so others can find it. It is nice that you have made it browser compatible.

All the best,

Jeff

@max-mapper
Copy link
Owner

@jeffbski oh yea good point, so:

  • if encoding is buffer, and data are buffers, it uses Buffer.concat() and returns a buffer
  • if encoding is string, and data are buffers, it will convert buffers to strings before concat, returns a string

however, the latter case, while possible, isn't recommended since you can just do the first case instead and then to buffer.toString(). string encoding is really intended for when your incoming chunks are strings. but I think i can probably fix multibyte char preservation for this edge case... cheers!

@jeffbski
Copy link

jeffbski commented Mar 5, 2014

Yes, it is one of those edge cases that would be tricky to understand after it happened, so if it can be made foolproof, all the better. I had hit this on a project and it had me confused for a bit.

@max-mapper
Copy link
Owner

@jeffbski fixed in 54444db. thanks for bringing it up

@jeffbski
Copy link

jeffbski commented Mar 5, 2014

Excellent!

@dmitrig01
Copy link
Author

Hahahahah now the two are even more similar! I'll submit a PR :-)

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

No branches or pull requests

3 participants