search

shell-pool / shpool

Think tmux, then aim... lower
Apache License 2.0
1.15k stars 20 forks source link

fix: protocol forward compat #86

Closed ethanpailes closed 2 months ago

ethanpailes commented 2 months ago

This patch switches us from using bincode, which has pretty terrible forwards compatibility properties to using msgpack (I think I did a switch in the previous direction in the past). bincode is faster, but we don't actually care about encoding speed since we are just encoding some tiny baby little headers at the start of a connection. This change also adds a bunch of #[serde(default)] annotations, which should take care of the second half of forwards compatibilty.

This means one last protocol version break before hopefully being able to evolve the protocol in a much more civilized manner.

I realized I was going to have to add a protocol field for #47, so this should be done first.

Fixes #85

Aetf commented 2 months ago

Thanks Ethan! Code LGTM.

Small nit on the commit message: let's keep commits formatted as per conventional commits, which will make generating changelogs a bit easier and requires less manual editing. Basically, we can have a BRAEKING CHANGE: footer, and optionally also a ! in the first line, something like

fix!: protocol forward compat

...

BREAKING CHANGE: why this is breaking etc and words that going into the changelog

Now, for whether this should be marked as a breaking change or not, IMHO it depends on whether we view the server and its protocol as a public API surface, and whether we allow version mismatches between the client and the server. If we say that's all implementation details, then I think it's OK for this to be a normal fix.

ethanpailes commented 2 months ago

We decided this doesn't count as a breaking change as the protocol is internal and users don't need to change any configs.