search

Shopify / rbi-central

MIT License
55 stars 36 forks source link

GraphQL::Schema#execute can return GraphQL::Query::Result #250

Closed bmulholland closed 3 months ago

bmulholland commented 3 months ago

Problem

https://github.com/Shopify/rbi-central/blob/main/rbi/annotations/graphql.rbi#L16 is missing at least one possible return type: GraphQL::Query::Result. https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/execution/interpreter.rb#L128

We use that in our app, for GraphQL subscriptions via ActionCable channels, ref: https://graphql-ruby.org/api-doc/1.11.5/GraphQL/Subscriptions/ActionCableSubscriptions

Context

bmulholland commented 3 months ago

Should it just become sig { params(query_str: String, kwargs: T.untyped).returns(T.any(T::Hash[String, T.untyped], GraphQL::Query::Result)) } ?

I could submit a PR for that

KaanOzkan commented 3 months ago

I'm surprised given this but if that's the type in runtime yes you can open a PR with that sig.

bmulholland commented 3 months ago

What's surprising about the line you linked?

KaanOzkan commented 3 months ago

execute returns multiplex which returns Interpreter#run_all which looks like it's returning an array. Yard documentation matches this too.

bmulholland commented 3 months ago

Ah yeah -- based on that it would be confusing. Maybe you missed the next line, which takes the first element from that array?

https://github.com/rmosolgo/graphql-ruby/blob/b7551b54d87355fd0d451cb335425272cbfdeb05/lib/graphql/schema.rb#L1305

KaanOzkan commented 3 months ago

Hmm wouldn't that element be a Hash or a Query?

But feel free to open the PR and I'll do a runtime test to make sure. As long as it's correct and doesn't degrade the more common usage of Hash it should be okay.

bmulholland commented 3 months ago

I filed an issue over at the GraphQL repo and they've actually replaced the return type to be exclusively a Result, not ever a Hash. https://github.com/rmosolgo/graphql-ruby/pull/4984

Following that, unless you see something I'm not, I'll change the type in my PR to always be a Result. Any concerns with that?

KaanOzkan commented 3 months ago

Thanks. Yes let's do Result.

bmulholland commented 3 months ago

Awesome. Updated the attached PR :)