jsonapi-suite / jsonapi_compliable

MIT License
20 stars 35 forks source link

Allow scopes access to runtime context #70

Closed richmolj closed 6 years ago

richmolj commented 6 years ago

In Rails, this means scopes would run in the context of the controller. So you can do things like:

default_filter :foo do |scope|
  scope.where(user_id: current_user.id)
end
richmolj commented 6 years ago

@wadetandy

wadetandy commented 6 years ago

Love the addition, though I'd generally prefer an api where the scope block had a context object instead of using instance_eval on the controller itself.

richmolj commented 6 years ago

@wadetandy OK, I switched and force-pushed. The biggest problem with yielding is accessing private methods, but I tend to think most would agree with you. Also, currently adapters do not have access to the runtime context...I think this is probably good and correct, and least for now, but moving to an argument sets us up for that if we need it in the future.

wadetandy commented 6 years ago

Fine with this version, though FWIW I was originally imagining keeping what you had before but just creating a dedicated runtime scope object with an object accessor:

scope_runtime = OpenStruct.new(context: resource.context)
@scope = scope_runtime.instance_eval(opts[:filter])

Then your block could access a context object or whatever other future needs without the arguments. I don't actually have a strong opinion either way here but this would preserve the original style a bit more if that's something you prefer.

richmolj commented 6 years ago

I considered that, but I think that's probably the most "surprising" option for a dev. I could see things making sense of "everything runs just like it would in the controller" or "there's an optional argument that is the controller", but "this runs in a special clean-room with access to the controller via context" seems the least-obvious option to me.

I also don't have strong opinions but I'm leaning towards this option just because 99% of the time you will not need to do this, it "just so happens" to match graphql syntax, and the adapter thing noted above.