handling Objects for streaming bodies

[tinovyatkin]

Koa currently throws while attempting to pipe an object mode stream into HTTP response.
This PR fixes that behavior allowing to serve that streams as JSON body.

Node.js 12.3 introduced a documented way to check that a stream in object mode stream, so, I think Koa must support such streams as first-class citizens out of the box.

[codecov]

Codecov Report

Merging #1405 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1405      +/-   ##
==========================================
+ Coverage   99.37%   99.39%   +0.02%     
==========================================
  Files           4        4              
  Lines         479      499      +20     
  Branches      128      136       +8     
==========================================
+ Hits          476      496      +20     
  Partials        3        3

Impacted Files	Coverage Δ
lib/response.js	99.38% <100%> (+0.01%)	⬆️
lib/application.js	98.44% <100%> (+0.21%)	⬆️

---

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 eda2760...1ca63ce. Read the comment docs.

[dead-horse]

I'm not sure there is a real world example that need this feature, and people can transform object streams to normal streams and used with koa. I'd prefer not implement it in koa core and leave it to community.

[tinovyatkin]

@dead-horse I think the wording transform object streams to normal streams is the key to understand what's wrong with not having object streams in Koa core from the initial Koa architecture point of view.

Object mode streams are normal streams, if under normal you mean that they are created by Node built-in streams module and have all events, functions, behavior of normal stream. The only difference here that chunk of data for non-object mode stream can be string or Buffer, and objectMode adds one more type of chunk - Object. Does it sounds similar to something, right? Yes, it's like standard Node response.end type expectation - it must be a string or Buffer.

The simplicity ctx.body = stream may confuse some developers, that Stream is just a third type of data that you can assign to the body. While in fact, it's another way to serve the same types data - it's sync response.end vs streaming response.pipe.

Let's see what Koa, as a framework, adding and promising to a developer here:

Sync (ctx.body = string, ctx.body = buffer; ctx.body = object)

Sync way to pass data simply delegates to standard response.end(data).
response.end can handle on it's own two types of data - string or Buffer. Passing anything other than that will result in an underlying error that string or Buffer value expected.
What Koa added here? Right, handling the third type of data - Object. Koa detects that ctx.body is an object not equaling to already supported string or Buffer, sets Context-Type to json and converts an object into supported data type - string. Everything perfect.

Streaming (ctx.body = stream)

Streaming way to serve response delegates to response.pipe. What response.pipe is doing internally with data? Well, it adds a listener for .on(data) and then delegates it to sync response.write.
response.write handles the data the same way as response.end. That means if data chunk from stream.on(data) is not a string or Buffer you will see exactly the same error message as passing something to response.end - string or Buffer value expected. So, why Koa on streaming way to deliver body doesn't do exactly the same magic as on sync way of delivering body? I mean, check the data type, handle Content-Type, stringify it.

So, what happened here, that Koa declares it's support for easy JSON handling, but while fully supporting two ways of delivering data - sync and stream, actually supports object-to-JSON conversion for response, for some unknown reason, only for the synchronous way of delivering response.

Object mode streams didn't feel like first-class citizens in early Node versions, so, we haven't seen so much real-world use cases. But now, with documented stream.readableObjectMode / stream.writableObjectMode, convenience of async iterators for await (const object of stream), new helper methods from streams module like finished and pipeline, they certainly will soon become way more popular as memory effective async way of handling big amounts of data. Real-word use today also exists as many streaming sources of data.

Supporting object for sync bodies is even simpler:

ctx.type = 'json'
ctx.body = JSON.stringify(object)

But Koa founders added that two lines into core, without delegating to the community. They just forgot (but remembered later on) to do the same on streaming way of delivering a response body, I assume due to lack of documentation or popularity of object mode streams.

So, this PR (20 lines of code) is not adding a new feature, nor handling non-normal streams - it closes the gap of original Koa promise - you can assign objects to response and Koa will do the rest of magic.

Contrary to merging this PR will be adding to documentation some warning:

Koa supports, like standard `http.ServerResponse`, two ways to set response body - sync and streaming. 
Also like a standard response, it supports two types of data - string or Buffer.
It also adds support for any object via JSON.stringify, but only for sync way of setting body.

[dead-horse]

Better to use module like https://github.com/stream-utils/streaming-json-stringify to do the stringify, also need to set content-type to json by default.

[tinovyatkin]

@dead-horse the module you suggesting wasn't updated in the last 3 years (so, not sure it will be maintained, and it uses deprecated new Buffer) and depends on two more external modules: readable-stream and json-stringify-safe (will lack consistency on stringifiyng with JSON.stringify for sync bodies). So, it will be way more non-normal stream and maybe a performance penalty.

I don't want to bring a lot of third-party dependencies in for just 20 lines of code.
Node.JS world is kinda moving from using a lot of one-liner modules practice for some serious reasons.

Also, my next PR will be exposing a way to stringify body (ie, create application.stringify = JSON.stringify.bind(JSON) and use everywhere when stringifying is happening, so, again, it's better to have control over stringifiyng in core.

[tinovyatkin]

This PR sets content-type to json for object mode streams by default, see changes to response.js file

[dead-horse]

Actually I'm not care about import some "one-liner module", many "one-liner module" have hundreds lines of test code. I don't want to re-implement these code in koa's code base.

streaming-json-stringify can be improved if the implement is outdated, I think @dougwilson and @jonathanong will be happy to accept pull requests with this module.

Also, my next PR will be exposing a way to stringify body (ie, create application.stringify = JSON.stringify.bind(JSON) and use everywhere when stringifying is happening, so, again, it's better to have control over stringifiyng in core.

I'm not sure this is really helpful, change stringify method seems useless to 99.99% of developers. Even though streaming-json-stringify can specific JSON.stringify by options.

[tinovyatkin]

@dead-horse There is open and approved PR in streaming-json-stringify since 2017. I don't think it's still maintained.

The code of my implementation is completely different and easier (no extra options, etc), so, it will be rewrite of streaming-json-stringify, not an update.

[dougwilson]

There is open and approved PR in streaming-json-stringify since 2017. I don't think it's still maintained.

I mean, Koa itself has an "open and approved PR since 2017" (#1011) so by that measurement Koa is not maintained, either :)

[tinovyatkin]

@dead-horse found very similar to mine implementation - https://github.com/big-kahuna-burger/json-stringify-trough. Are you OK to introduce such dependency?
100% test coverage, no more dependencies, but author don't have many published packages and this one is not very popular.
Again, I tend to see any external dependency as a security risk and consider this functionality as core Koa function.

[tinovyatkin]

Nope, it's not exactly the same. The main difference between my implementation and any module I've found on the internet that in case of no data my implementation will output empty array (that's correct implementation of empty stream in my opinion and will be parsed well if content-type is json) while the rest outputs nothing (and so, JSON aware client like request or superagent will throw on invalid JSON string body).
Are you sure we need to move this functionality into an external module? I can publish one, but, again, it's just one constructor call, 15 lines total.

[tinovyatkin]

As I wrote above, we soon will see more libraries adding support for object stream. Here another example of recent update of very popular open source library adding objectMode streams support.

[jonathanong]

this belongs in a separate module. i can give you access to https://github.com/stream-utils/streaming-json-stringify if you want @tinovyatkin

[tinovyatkin]

@jonathanong I would love to refactor streaming-json-stringify and reimplement this PR using that module. Could you give me access, please?

[jonathanong]

done

[lagden]

@tinovyatkin you can use http://oboejs.com/
it is a good lib!