Init callback return option :subscribe_to looks too restrictive

[bamorim]

I was reading the docs and realized that the type for the :subscribe_to option on the callback may be too restrictive.

Here it says that the value for subscribe_to is either [module | {module, subscription_options}].

This tripped me out because I was expecting something like GenServer.server() or GenServer.name(), because I was using a via tuple to subscribe_to, because I have dynamic GenStage flows that start and stop according to changes in the database.

Am I doing something wrong or could the type actually be GenServer.server()?

I started looking for the usages of that value, and the only place I could find is here, where it calls GenServer.whereis/1, which argument is of type GenServer.server().

If the expected type was actually supposed to be GenServer.server() I can easily open a PR for that. If we should not use anything other than module() then sorry for bothering you.

(The only problem is that possible names include tuples like {:global, term()}, then we would need to do extra distinction to not mess up and think that the GenStage is :global and term() is option. Another problem is {atom(), node()}. But we can just pattern match those first (before matching on {to, opts}).

Thanks for your time and sorry if bothering for no reason.

[josevalim]

@bamorim it is definitely GenServer.server. There is an ambiguity with {:global, term} and {atom, node} but AFAIK it has always existed, even in Erlang. So it is an "issue" rather with Elixir and not really with GenStage. So a PR is welcome!

[bamorim]

Whoa, thanks for that fast reply. I'll work on a PR and submit at no later than next weekend.

Should I handle (and add tests) {:global, term} when term is a list (which would conflict with the current pattern match) as well? Also, I was thinking about adding tests that we can start with all possible scenarios {:global, term}, {:via, module(), term()}? Just so future changes don't break those and we are actually accepting all possible GenStage.server() values.

[josevalim]

Oh, I see the ambiguity now! I think we should go for atom | pid | {GenServer.server, opts}. WDYT?

[bamorim]

That is great! It is not that much boilerplate for complex use cases (such as mine) and removes the ambiguity (making tests and the spec easier to follow and understand).

[bamorim]

What should I do for cases where people are actually sending a GenServer.server() directly (without the opts). If they are sending via tuples, it is currently working (even though it is not the "documented" behavior). So I think we should either add a deprecation warning or just accept it anyways. Your thoughts?

[josevalim]

Let's deprecate just to be safe. :)

[bamorim]

About the deprecation message, I was thinking something like that:

:subscribe_to value ~tp~n is deprecated because {:global, term()} can cause ambiguity with {atom(), list()} used for passing options to subscribe_to. Change to {~tp~n, []} instead.

This is a little bit verbose, but I tried to be as explicit as possible. Suggestions on how to improve this?

[bamorim]

Also, technically speaking, {:via, module(), term()} does not cause ambiguity, should we also check for that one? Or should we make the type atom | pid | {:via, module(), term()} | {GenServer.server, opts}. Maybe this makes things too complicated. One option is to just say that passing {:global, term()} and {:via, module(), term()} is deprecated, give information on how to change and do not explain about ambiguity.

[josevalim]

@bamorim actually, our previous spec did not actually support anything other than a module at the root, so I think we are fine. We can also just update the spec and not change the behaviour, so we don't break anyone accidentally relying on the behaviour for now. We can introduce the deprecation warning later on - if necessary.