Return string rather than Buffer on response

[Cherry]

In previous versions of Dockerode, this was expected behaviour (to an extent, but a stream was generally expected prior to this issue). 2.5.8 broke this, and now returns a Buffer when the parseJSON fails. This PR returns the stringified buffer even when this fails, rather than the Buffer object.

[Cherry]

If this is a change that was intentional, then it should probably bump SemVer for a major change. This is not backwards compatible.

[apocas]

This was introduced in:
#97

Indeed this got pushed into the wrong version, causing this backward compatibility issue.

[Cherry]

@apocas Thanks for the info. What's the best course of action here? Release a 1.0.9 of docker-modem with this PR to fix the backwards incompatible change, and then schedule a SemVer bump for the release of #97?

[apocas]

Yes lets revert this and then bump the version.
I also need to bump dockerode due to other changes, so everything will match nicely.

[Cherry]

Perfect, thanks. 👍

[githubsaturn]

What was the thought behind this? Docker logs are not utf8 Strings! Buffer.toString() uses utf8 by default:
https://nodejs.org/api/buffer.html#buffer_buf_tostring_encoding_start_end

By blindly converting a Buffer to a utf8 string, junk characters are introduced in the beginning of each line. Getting logs from container now contains junk chars if it's not a stream.

@Cherry / @apocas had you tested this before merging? This should be reverted back to Buffer. Otherwise, demuxing is impossible. I'll create a new PR converting this back to Buffer unless there is a way for demuxing that I cannot think of?

[Cherry]

@githubsaturn This PR was solely issued to restore the behaviour in Dockerode 2.5.7, that was broken in 2.5.8. Dockerode always returned strings prior to #97.

I'm not against using a Buffer and would +1 this change if a new major version was issued as per SemVer, since this would be a breaking change, but this unannounced breaking change broke our build in the past, thus this PR was created to restore the original behaviour.

See apocas/dockerode#456 for more discussion.

[githubsaturn]

@Cherry

I'd argue that this broke the backward compatibility because of a bugfix. Keeping a buggy implementation for the sake of not changing the behavior is less than desirable IMHO.

Anyways, this is what I suggest:

• Bump a major change for docker-modem and return Buffer.
• Update dockerode to use the newly updated docker-modem (that returns Buffer). However, we'll add an optional arg to .logs for the encoding. By default it can use utf8 - which we know is wrong. But at least, users can forcefully change this to hex, for example, to get the correct format out.

@apocas
Are you okay with this proposal?

[Cherry]

I agree it's less than desirable, but given it was this way for years, there's a lot of code in the wild that relies on it.

Nevertheless, I'd +1 bumping a major version for dockerode and docker-modem for fixing this.

[apocas]

This weekend I will release new major versions for both of projects, fixing this and improving the promise based interfaces.