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

Field Extensions (second try) #1758

Merged
merged 17 commits into from
Aug 14, 2018
Merged

Field Extensions (second try) #1758

merged 17 commits into from
Aug 14, 2018

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Aug 9, 2018

Trying the same notion from #1411:

  • Programmatically modify field resolution on a field-by-field basis
  • Low runtime overhead
  • Abstract away Promise & friends

TODO:

  • add filters that can:
    • add arguments to field definition
    • intercept args before calling methods
    • modify return values
    • pass arbitrary data between before_ and after_ steps
  • Migrate connections to use filters
  • Migrate scope: to use filters
    • As with filters, raise when scope is used with a legacy resolver (require scope: false to opt out)
  • Clean-up:
    • Rename? Is filter a good name?
    • Extract Field#to_options to a separate PR to reduce this diff refactored to use .add_field instead
    • Settle on a consistent API for registering and using filters
    • Refactor Field#with_filters to not create a giant stack trace (as in Change call_hooks to be iterative rather than recursive #1738)
  • write a guide
    • Document that it's not supported for function: or legacy field:
  • Make sure that promises, errors, nil and skip are all handled by the library, let's wait and see, because you might want to handle those things, I don't want to hide them all.
  • Add a dedicated hook to modify the field definiton

@rmosolgo rmosolgo self-assigned this Aug 9, 2018
@rmosolgo rmosolgo mentioned this pull request Aug 9, 2018
@rmosolgo
Copy link
Owner Author

rmosolgo commented Aug 9, 2018

Wow, amazingly this seems like it might work. Still lots of clean-up to do though.

@rmosolgo rmosolgo added this to the 1.8.8 milestone Aug 9, 2018
@rmosolgo rmosolgo changed the title Field Filters (second try) Field Extensions (second try) Aug 10, 2018
@rmosolgo
Copy link
Owner Author

I renamed it to FieldExtension to capture that it does more than just filtering inputs. I guess it's OK but I'm definitely open to a better name.

@jcbpl
Copy link

jcbpl commented Aug 13, 2018

"Extensions" makes sense to me, it's similar to ActiveRecord's extending method. The other name that comes to mind for composing fields out of behaviors is "traits," but I think one advantage of "extensions" is that you'd sort of expect to have to pass it a module.

class_based_api: true
---

{{ "GraphQL::Schema::FieldExtension" | api_doc }} provides a way to modify user-defined fields in a programmatic way. For example, Relay connections may are implemented as a field extension.
Copy link

Choose a reason for hiding this comment

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

Relay connections may are implemented as a field extension

Typo.

Copy link
Owner Author

Choose a reason for hiding this comment

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

oops, thanks!

@rmosolgo rmosolgo merged commit d483c91 into master Aug 14, 2018
@rmosolgo rmosolgo deleted the field-filter-instances branch August 14, 2018 19:29
@rmosolgo
Copy link
Owner Author

This has an odd side-effect, since connections are now implemented via class-based API, Relay::BaseConnection#arguments is now a plain Ruby hash of symbols, not a Query::Arguments instance. This means it doesn't work with strings or camelized names anymore.

It's a bit of a breakage that i wasn't expecting 😅

@rmosolgo
Copy link
Owner Author

This is nice, but the compatibility issue is no joke. I'm going to revert it from 1.8.x and put it in 1.9.x instead.

@rmosolgo rmosolgo restored the field-filter-instances branch August 20, 2018 21:29
@rmosolgo rmosolgo removed this from the 1.8.8 milestone Aug 20, 2018
rmosolgo added a commit that referenced this pull request Aug 20, 2018
rmosolgo pushed a commit that referenced this pull request Sep 13, 2018
@rmosolgo rmosolgo deleted the field-filter-instances branch October 5, 2018 19:52
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