Multi-character and multi-byte delimiter support & partial use of es6 (nodejs requirement latest 6.x branch)

peterhaldbaek / delimiter-stream

Splits a stream into chunks separated by delimiter

MIT License

4 stars 3 forks source link

Multi-character and multi-byte delimiter support & partial use of es6 (nodejs requirement latest 6.x branch) #3

Closed yarrysmod closed 6 years ago

yarrysmod commented 6 years ago

Added test cases where the multi-byte and multi-char delimiters are in use
Switched from string concatenation to a buffer for safer delimiting (especially the case for incomplete data-event emission)

peterhaldbaek commented 6 years ago

Sweet! Thanks for the contribution, I really like the Buffer part. I guess it makes it a lot faster as well? Not sure when I have the time to review it properly but hopefully it will be within the next week. 👍

yarrysmod commented 6 years ago

The benefit of leaving it as a buffer means that the data is only transformed to a String when needed.

In the first commit I've implemented the buffer but relied on the String split.. it turned out to be a bad idea when the delimiter character was multi-byte or if it was a multi character string.

So String split would be out of question, I have switched to a different method where I iterate through the received buffer and try finding the delimiter buffer sequence inside the chunk and then split the messages inside the chunk to an array containing the buffers.

That way, in case I received an incomplete multi-byte delimiter at the end of a chunk, it would not be corrupted to a � and when the next chunk arrived would be properly completed for splitting.

peterhaldbaek commented 6 years ago

That makes sense. My usecase for creating this was much simpler as I was parsing ANSI characters and the delimiter was a single character. Your addition improves the stream a lot so I see no reason to reject it right now. If anything I would probably prefer a single commit but I can probably fix it up on my end.

peterhaldbaek commented 6 years ago

Is the package-lock.json file necessary? Been away from Node for a while but reading up on it it seems like it should not be added for libraries. See https://stackoverflow.com/q/44206782 (especially the comments).

yarrysmod commented 6 years ago

The package-lock.json ensures that the installed dependencies stay the same for development and deployment / whenever an "npm install" is executed.

That way there are no discrepancies between installations, it made sense to me

peterhaldbaek commented 6 years ago

Yes, I see the reasons for having it. I was just reading up on different opinions on the matter and started wondering whether it was needed or not. Since other projects depending on this will not be affected by the lock file I am okay with leaving it as it makes it easier to reproduce issues (as you argue). I will keep it.

yarrysmod commented 6 years ago

An issue right now would be that the changes with the new language features would drop support for anything below Node 6.x

Node 4.x end of life will be reached by the end of the month. https://github.com/nodejs/Release

peterhaldbaek commented 6 years ago

Yes, I noticed that but don't think it is an issue. As you mention Node 4 is very close to EOL so no matter what it is time to move on. We could also make the version 2.0.0 as it could be considered a breaking change but it seems a little overkill.