Return string rather than Buffer on response

apocas / docker-modem

Docker Remote API network stack driver.

Apache License 2.0

235 stars 112 forks source link

Return string rather than Buffer on response #100

Closed Cherry closed 5 years ago

Cherry commented 5 years ago

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 commented 5 years ago

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

apocas commented 5 years ago

This was introduced in: https://github.com/apocas/docker-modem/pull/97

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

Cherry commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

Perfect, thanks. 👍

githubsaturn commented 5 years ago

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 commented 5 years ago

@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 https://github.com/apocas/docker-modem/pull/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 https://github.com/apocas/dockerode/issues/456 for more discussion.

githubsaturn commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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