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

Add lazy eval #92

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add lazy eval #92

wants to merge 1 commit into from

Conversation

v2kovac
Copy link

@v2kovac v2kovac commented Mar 30, 2024

Follow up to this pr: #89

Added a lazy_eval method to chain methods together.
I called it lazy_eval instead of lazy to avoid breaking changes.
Follows Enumerator::Lazy with eager and force methods that only become available after lazy_eval is called.
eager tells the loader to stop accumulating methods in the lazy chain and force a real evaluation on the next call.
force tells the loader to sync immediately (kind of redundant, you shouldn't use lazy_eval if you're gonna force)

In graphql, a sync is implicitly done at the correct time so eager isn't required to break the lazy chain. I automatically disable lazy chaining after a sync. This allows a nice looking:

def user_name
  user.lazy_eval.name
end

But in non graphql contexts, you'd have to do:

def lazy_user_name
  user_lazy.lazy_eval.name.eager
end

The code is relatively easy to understand, and the api is fairly analogous to Enumerator::Lazy, but I understand if you think it's too complex. Let me know if you have any thoughts, was fun to work on regardless.

@exAspArk
Copy link
Owner

exAspArk commented Apr 2, 2024

Thank you for submitting it! I'll try to find time to review it over the weekend

@exAspArk
Copy link
Owner

Sorry for the delay here, and thank you for submitting this PR! 🔥

What do you think about isolating this logic to GraphQL only? My current thinking:

  • It might not be that useful for non-GraphQL use cases
  • Adding it to BatchLoader will increase complexity and make maintenance harder
  • Maybe for GraphQL-only, the implementation could be much simpler

Just as an idea, user.lazy.id returning a new BatchLoader::GraphQL somehow that can be synced by GraphQL automatically and point to the "same" @batch_loader? Maybe even creating a new "lazy" GraphQL class with method_missing to return BatchLoader::GraphQL?

I called it lazy_eval instead of lazy to avoid breaking changes.

To clarify, is it to avoid breaking changes if someone uses Enumerator::Lazy?

@jesseduffield
Copy link

jesseduffield commented Oct 16, 2024

Just chiming in: I just came looking in this repo for the ability to call .then on the loader object and I'm not using graphQL: I'm using Blueprinter (i.e. classic rest API with serializers).

My use case is the same: often I've got multiple fields in my serializer that each make use of the same underlying query and I don't want to have to run those queries twice.

For the record, I would personally prefer using .then so I can just pass a block that describes what I want to do with the value once it's synced. I don't care if the method is actually called 'then' or not (I'd happily use 'batch_loader_then' to avoid name collisions), I just want some way of specifying what I want to do next with the value.

I suspect this would be simple to implement and it would benefit both GraphQL and non-Graphql users.

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