Phoenix.Presence.get_by_key/2 return values and documentation are inconsistent

[sokolnickim]

Environment

Elixir: 1.12 Phoenix: 1.5.8, it looks like it's applicable to all versions since the function was introduced.

Problem

According to documentation, get_by_key/2 in Phoenix.Presence should return "the map of presence metadata for a socket/topic-key pair", and the further Examples section says it "Uses the same data format as list/1. This would be congruent with the specs, which state both get_by_key/2 and list/1 return the type presences(), defined as:

presences() :: %{required(String.t()) => %{metas: [map()]}}

On the other hand, the actual code in the example shows the function returning a list:

iex> MyPresence.get_by_key("room:1", "user1")
[%{name: "User 1", metas: [%{device: "Desktop"}, %{device: "Mobile"}]}]

Which looks more like what we would get if we took the presences() format and applied Map.values/1 to it.

Finally, the actual behavior of the function seems to be a cross between both of these conflicting formats. For existing entries, it returns the map containing the metas key, in other words what the example code shows, without the surrounding list (which is superfluous anyway, since there will only ever be one entry for one user). For missing entries, it returns the empty list.

Among all these conflicting sources of information, it isn't clear what should even be the correct behavior, that's why I didn't even attempt to create a pull request. I can see three reasonable solutions.

Solutions

The first is to be consistent with list/1 and to follow the spec. The example code should then read %{"123" => %{name: "User 1", metas: [%{device: "Desktop"}, %{device: "Mobile"}]}}, and non-existent entries should return an empty map. The drawbacks though are that it returns useless data (the outer map), empty maps are unpleasant to use in pattern matches and most importantly, it would break all existing code, since all returned values would change.

The second would be to stick to the current behavior and rewrite the spec and docs. The only quirk here is that an empty list is still returned for non-existent entries, which would have to be documented and justified. It also has the disadvantage of being a truthy value, so if all we are interested in is whether the user is present, we can't do if get_by_key(topic, id) ....

The third solution would be my favorite, and it would be to just change the return for non-existent entries from an empty list to a nil. That brings it in line with the convention of the rest of the language and should bring minimum breakage. Both docs and specs would need to be changed, but we should gain clarity in the process.