-
Notifications
You must be signed in to change notification settings - Fork 82
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
fix: Support lazy objects #17
Conversation
- Adds field extension that supports lazy object resolution and type assignment - Adds field extension to _entities field
- Scopes lazy object class within let block - Resolves other rubocop issues
… lazy-object-support
Great work @cmschuetz! @rylanc anything blocking from merging this in? |
… lazy-object-support
fixed the builds |
Tested on a small artificial app with https://github.com/shopify/graphql-batch - works. @noaelad @rylanc could you find some time to review and merge it. |
Apologies for the delay! I'll take a look at it today |
def after_resolve(value:, context:, **_rest) | ||
synced_value = | ||
value.map do |type, result| | ||
[type, context.query.schema.sync_lazy(result)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused about the need to call sync_lazy
in after_resolve
, from reading the docs it seems like after_resolve
should have been called after all the lazy objects have synced, however that does not seem to be the case. I pulled this branch down and played with it a bit and it does seem like the objects haven't been "lazy loaded" until this point, so I agree this is needed but just not sure why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also initially thought that the lazy values would be resolved at this point which led me to reach for this solution. My best guess is that the docs mean that there are are no more lazy evaluations in flight for the given field. E.g. In the context of graphql-batch
, all keys have been collected for the given loader and it's 'ready' to be resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Played around with this code and it works nicely, thanks for contributing a feature! Curious if you can answer my question inline because I couldn't piece this together.
## [0.4.1](v0.4.0...v0.4.1) (2019-10-12) ### Bug Fixes * Support lazy objects ([#17](#17)) ([68b0b9a](68b0b9a))
🎉 This PR is included in version 0.4.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This introduces a mechanism for resolving lazy objects before setting their type on the query context. Previously, type resolution didn't work because the lazy object was being set instead of its resolved value.
I believe
after_resolve
is the correct hook to introduce this behavior as objects will be 'synced', but curious if there are other strategies for accomplishing this