Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate returning .resolve(...) dataloader requests #4807

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

rmosolgo
Copy link
Owner

This was previously supported, but it shouldn't be -- it makes Dataloader dependent on the lazy-resolve system. I'm interested in working on that system so I'd like to disentangle them first.

@rmosolgo rmosolgo added this to the 2.2.7 milestone Jan 25, 2024
@rmosolgo rmosolgo merged commit 02dd62b into master Jan 26, 2024
12 checks passed
@rmosolgo rmosolgo deleted the deprecate-dataloader-lazy-resolve branch January 26, 2024 20:09
@rmosolgo rmosolgo mentioned this pull request Jan 29, 2024
20 tasks
@Bajena
Copy link

Bajena commented Apr 22, 2024

Hi @rmosolgo, I'm trying to figure out how to migrate from the deprecated .request to .load calls in my code.

We're currently using a dataloader source is Apollo's resolve_reference (https://github.com/Gusto/apollo-federation-ruby?tab=readme-ov-file#reference-resolvers) and the things are batch loaded correctly. However, as soon as I switch to load, each record gets loaded separately.

Is the deprecation there, because there'll be some additional work done in the future? Or maybe we're doing something wrong? 🤔

@rmosolgo
Copy link
Owner Author

additional work done in the future

Yes, my goal is to move GraphQL-Ruby's "lazy resolution" outside the core runtime, so that it doesn't add overhead to schemas that don't use it. (Lazy resolution is a pre-Dataloader approach to batch loading, used by GraphQL-Batch among others: https://graphql-ruby.org/schema/lazy_execution.html). Dataloader was hooked up to that but I think it doesn't have to be.

I looked over the docs there but I don't quite see the usage of .request. Could you share some example code where .request is used? I'd be happy to look for a way to migrate it to .load (or perhaps .load_all in this case?) and make sure there's a way forward.

@utay
Copy link

utay commented Apr 23, 2024

Hello, same issue here.

We have to use .request in federation's self.resolve_reference:

module Baseball
  class CardType < Base::Object
    ...
    def self.resolve_reference(reference, context)
      context.dataloader.with(RecordLoader, Card, column: :asset_id).request(reference[:assetId])
    end

As resolve_reference gets called for each record separately, .load doesn't batch queries and .load_all can't be used.

@Bajena
Copy link

Bajena commented Apr 23, 2024

@rmosolgo I only found request here: https://graphql-ruby.org/dataloader/overview.html

Also, thank you for a very detailed explanation. I'll keep the deprecation warning for now and will be observing the development 🍿 :)

@utay
Copy link

utay commented Apr 23, 2024

Actually I was able to make it work with resolve_references and .load_all!

resolve_references (plural) is not documented in README but it's working well. By the way it was introduced after @rmosolgo's comment, thanks!

@rmosolgo
Copy link
Owner Author

👍 Thanks for sharing what you found, @utay. I was about to make the same suggestion. It's here in the source:

https://github.com/Gusto/apollo-federation-ruby/blob/51af77bce4e48e864e97b849053f1e5992edcd6a/lib/apollo-federation/entities_field.rb#L62-L63

You could use .load_all in that method, for example:

def self.resolve_references(references, context)
  asset_ids = references.map { |r| r[:assetId] }
  context.dataloader.with(RecordLoader, Card).load_all(asset_ids) 
end 

Let me know how it goes if you give it a try!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants