Skip to content
This repository was archived by the owner on Apr 8, 2020. It is now read-only.

Rescue from lazy resolution#14

Merged
exAspArk merged 4 commits intoexAspArk:masterfrom
DamirSvrtan:rescue-from-lazy-resolution
Dec 17, 2018
Merged

Rescue from lazy resolution#14
exAspArk merged 4 commits intoexAspArk:masterfrom
DamirSvrtan:rescue-from-lazy-resolution

Conversation

@DamirSvrtan
Copy link
Copy Markdown
Contributor

As commented in #12, I've added work on top of your PR #13 .

Let me know if that looks good to you!

@DamirSvrtan
Copy link
Copy Markdown
Contributor Author

Oh brilliant, it's not working with Graphql Ruby 1.7...

Comment thread spec/fixtures/schema.rb
@@ -46,26 +53,6 @@
query QueryType
mutation MutationType
Copy link
Copy Markdown
Owner

@exAspArk exAspArk Dec 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DamirSvrtan I know why. I forgot to add use BatchLoader::GraphQL in the GraphQL 1.7 schema in this PR #13, my bad. Took me a while to debug and understand why it fails :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant, thanks for the help! I'll add it!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One interesting thought dough - if you forgot to add use BatchLoader::GraphQL, then it was resolved as a regular field, right? Shouldn't it then be caught by the normal field resolve proc?

Copy link
Copy Markdown
Owner

@exAspArk exAspArk Dec 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good question. It should but, unfortunately, it fails somewhere on the GraphQL-Ruby level when it tries to detect whether the object is lazy or not. And when it performs the check, BatchLoader is being resolved with an exception somewhere outside GraphQL instruments. I.e. devs should handle the exception in the BatchLoader itself in this case or improve the instrospection in GraphQL-Ruby (when it checks a lazyness)

Comment thread lib/graphql/errors.rb Outdated

private

def wrap_proc(object, arguments, context, old_resolve_proc)
Copy link
Copy Markdown
Owner

@exAspArk exAspArk Dec 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😻

we might want to rename old_resolve_proc arg to something else (proc, old_proc, etc.) to avoid any confusions (old_resolve_proc vs old_lazy_resolve_proc)

@DamirSvrtan
Copy link
Copy Markdown
Contributor Author

@exAspArk I've updated the PR!

@exAspArk exAspArk merged commit dd1d4e3 into exAspArk:master Dec 17, 2018
@exAspArk
Copy link
Copy Markdown
Owner

Released in v0.3.0, thank you! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants