absinthe-graphql / absinthe

The GraphQL toolkit for Elixir
http://absinthe-graphql.org
Other
4.29k stars 527 forks source link

Add optional run_docset callback to Absinthe.Subscription.PubSub behavior #1305

Closed michaelcaterisano closed 7 months ago

michaelcaterisano commented 8 months ago

Per https://github.com/absinthe-graphql/absinthe/issues/1293 and https://github.com/absinthe-graphql/absinthe/pull/1295#issuecomment-1968844198, this PR adds an optional run_docset callback to the Absinthe.Subscription.PubSub behavior. This function is responsible for resolving subscription documents and publishing data to each topic. If no callback is provided by the behavior implementation, Absinthe's default run_docset function is called.

cc: @benwilson512 @bryanjos

michaelcaterisano commented 8 months ago

@benwilson512 One potential improvement here might be to make it easy for implementors of run_docset to grab the pipeline used in the default run_docset function:

pipeline =
          doc.initial_phases
          |> Pipeline.replace(
            Phase.Telemetry,
            {Phase.Telemetry, event: [:subscription, :publish, :start]}
          )
          |> Pipeline.without(Phase.Subscription.SubscribeSelf)
          |> Pipeline.insert_before(
            Phase.Document.Execution.Resolution,
            {Phase.Document.OverrideRoot, root_value: mutation_result}
          )
          |> Pipeline.upto(Phase.Document.Execution.Resolution)

        pipeline = [
          pipeline,
          [
            result_phase(doc),
            {Absinthe.Phase.Telemetry, event: [:subscription, :publish, :stop]}
          ]
        ]

That way callback implementors don't have to recreate this bit. What do you think?

Something like:

pipeline = Absinthe.Subscriptions.Local.pipeline(doc, mutation_result)

EDIT: Done in https://github.com/absinthe-graphql/absinthe/pull/1305/commits/90f868eaf90c897314c27ef9af390c2f35a27a38. Let me know if there's a better place to put this.

michaelcaterisano commented 8 months ago

@benwilson512 Let us know if you have any feedback about this approach. We're using this in our fork and it's a great solution for us.

michaelcaterisano commented 8 months ago

@benwilson512 Friendly bump, any chance you have time to take a look? We'd love to get something like this merged and in the next release so we can move off of our fork.

benwilson512 commented 8 months ago

Hey @michaelcaterisano apologies, ended up getting COVID. Still recovering, and now the kids have it. Gonna put a calendar invite to myself to review this in like a week.

michaelcaterisano commented 8 months ago

Hey @michaelcaterisano apologies, ended up getting COVID. Still recovering, and now the kids have it. Gonna put a calendar invite to myself to review this in like a week.

Sorry to hear! Hope ya'll feel better soon.

michaelcaterisano commented 7 months ago

@benwilson512 Friendly bump. Hope you're feeling better!