refactor shim to support nodejs 10.x, support nodejs10.x runtime by default

[skabbes]

After referencing this PR I was pretty sure I knew what the difference between node8 and 10 was. The data events changed significantly between those 2 versions, I am not surprised the out of date byline code vendored in broke.

I refactored the shim to use the readable event and Stream#read method, which is supported in both node 8 and node 10.

The "line un-chunker" still processes newlines in a streaming fashion as fast as possible, but no longer supports \r delimited lines. And also passes a Buffer directly to JSON.parse instead of decoding to a string first.

[CLAassistant]

All committers have signed the CLA.

[skabbes]

Hmm, I already signed the CLA a looong time ago - but I think it was lost in the "great commit purge" a while ago :)

[skabbes]

The failing codeclimate is "code complexity" which I think overall has been improved, particularly with byline removed, which was a pretty legacy Streams implementation.

I'm claiming reduction in complexity!

[tj]

I might be reading this wrong, but should this reset to 0 as well since we're resetting the chunks to []?

[skabbes]

Maybe I'll comment this a bit better, and add a new commit.

chunks is an array of buffers that stores all previous data chunks (Buffer) that didn't have a newline yet (so you can't turn them into a line)

Since the current chunk can have multiple lines inside of it, startPos is an index into the current chunk (which is a Buffer) not chunks (which is an array of Buffers).

For a chunk like ['a', 'b', 'c', '\n', 'd', 'e', 'f'] Start pos will be 0 (aka 'a') then 4 (aka 'd'). The the slice ['d', 'e', 'f'] will be saved in chunks until we receive the next chunk of data.

chunks = [Buffer ['d', 'e', 'f']];

[tj]

oops I misread that, thanks for clarifying!

[tj]

LGTM! I'll try it out this weekend thanks man

[tj]

I tried with the node-basic, but I'm getting an unmarshal error:

  Aug 25th 12:27:53pm INFO 2019/08/25 11:27:53 error decoding input: json: cannot unmarshal number into Go struct field input.id of type string

[skabbes]

ergh, sorry - I tested the line chunk parsing, but not the interaction with the whole proxy. I stupidly changed string -> number since Map supports that, and it felt more natural. The Go side of the proxy did not like that.

How do you test this? I can run it through that same test, sorry for wasting your time :(

[skabbes]

Hey TJ, I’m sorry about the broken PR, it was not up to my normal bar for quality.
I did get a version of go-bindata and peg installed so I can build and test up full stack. I’ll update the PR and also dogfood this on my production load. Just one question:

Do you want node10 to become the default runtime (even for people who have previously been using 8 ). This would affect people who were running their services on node8 previously, but for some reason they aren’t compatible with node10 (maybe native code?). That seems to be the safest option, but I think new projects will automatically pick node8 as the default - (which I presume is unwanted).

for other languages, go, crystal, etc. This should not be a huge change. Not sure what your policy has been in the past for these pseudo breaking changes.

[skabbes]

closing in favor of: #778 this change avoids anything that could be breaking, node10 support is opt in.