Replace tuple style supervisor child_spec with map on cowboy adapter #266
Conversation
|
I see CI pipeline failed on dialyzer check because current Should I suppress dialyzer warning for this? |
Does the code allow for returning a map, though? |
|
Yes, for newer version of |
|
Looks like this PR should also update Ranch |
| @@ -42,13 +42,26 @@ defmodule GRPC.Server.Adapters.Cowboy do | |||
| {:ranch_tcp, :cowboy_clear} | |||
| end | |||
|
|
|||
| {ref, mfa, type, timeout, kind, modules} = | |||
| :ranch.child_spec(ref, transport, trans_opts, protocol, proto_opts) | |||
| case :ranch.child_spec(ref, transport, trans_opts, protocol, proto_opts) do | |||
There was a problem hiding this comment.
If we update ranch, we won't need this case
There was a problem hiding this comment.
If we update :ranch, then the tuple version would never match afterward, the dialyzer would see it as warning
There was a problem hiding this comment.
Yeah, because we won't need to support the tuple version :)
There was a problem hiding this comment.
we can't update ranch because cowlib still dependent on version 1.8.0 :(
mix.exs
Outdated
| @@ -44,6 +44,7 @@ defmodule GRPC.Mixfile do | |||
| # but we can't depend on an RC for releases | |||
| {:gun, "~> 2.0.1", hex: :grpc_gun}, | |||
| {:cowlib, "~> 2.11"}, | |||
| {:ranch, "~> 2.0", override: true}, | |||
There was a problem hiding this comment.
Why do we need the override?
We can't publish the package with the override here :(
There was a problem hiding this comment.
because cowlib still reference to "1.8.0", this means this approach need to suppress warning from dialyzer on no_match for :ranch.child_spec
There was a problem hiding this comment.
I'll revert it back
There was a problem hiding this comment.
Ok, so we can't update ranch after all.
Best approach is to revert to the case implementation and ignore the dialyzer warning for that specific line.
There was a problem hiding this comment.
Hex blocks packages that have override: true for any dependencies, because override should only be used at the top level
| @@ -42,13 +44,24 @@ defmodule GRPC.Server.Adapters.Cowboy do | |||
| {:ranch_tcp, :cowboy_clear} | |||
| end | |||
|
|
|||
| {ref, mfa, type, timeout, kind, modules} = | |||
| :ranch.child_spec(ref, transport, trans_opts, protocol, proto_opts) | |||
| case :ranch.child_spec(ref, transport, trans_opts, protocol, proto_opts) do | |||
There was a problem hiding this comment.
Could you add a comment here and on the dialyzer warning explaining why these pieces of code are here?
There was a problem hiding this comment.
It's just a no_match/never match warning since :ranch version 1.8.0 will always return a tuple lilke current implementation, while new version (2.x.x) will always return map.
So, even though we update ranch the warning still exist because it'll just return one type of child_spec, a tuper or a map
And turned out we can't update :ranch because :cowlib still dependent on version 1.8.0
I think I'll close my PR, since it'd not be generic use cases for having support for both of supervisor child_spec
I'll use my forked version for my plugin
Thanks Paulo
PR for #265 @polvalente