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

[POC] Allow Graphiti usage for api requests to be optional#53

Open
caseyprovost wants to merge 35 commits intographiti-api:masterfrom
caseyprovost:master
Open

[POC] Allow Graphiti usage for api requests to be optional#53
caseyprovost wants to merge 35 commits intographiti-api:masterfrom
caseyprovost:master

Conversation

@caseyprovost
Copy link

@caseyprovost caseyprovost commented Nov 20, 2018

Goal

At the moment this library does not play nice when using an API already built on JSONAPI Rails. This is because it includes this gem and overrides some default behavior. It would be AMAZING if this library could run along side the jsonapi-rails gem!

How (options & ideas)

  • Look for an instance variable defined in the controller and render conditionally (something like @graphiti_controller == true
  • Introduce libary configuration that is used at render time. (Something like Graphiti.config.strict) which would only manipulate requests/params/etc if the current route was in Graphiti's api_namespace.
  • Alter Graphiti so that it only manipulates requests under it's defined namespace.

Approach

I opted for altering the library code to make its inclusion in projects a little easier. The idea here is that if someone is creating an API or a new version and are already using some of the dependent gems in this project...there is no conflict (other than gem version potentially).

Problems With Approach

  • Graphiti currently only supports a singular api_namespace. So if someone was building an API under /v1 and then added another API version under /v2 which also used Graphiti...then the second version would run into a problem.

@richmolj richmolj added the to do label Dec 10, 2018
Copy link
Contributor

@richmolj richmolj left a comment

Choose a reason for hiding this comment

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

I think we need the MIME type to be configurable, where jsonapi is the default and graphiti is the override. But otherwise this looks great - apologies for the delay, I really appreciate your hard work and follow-though on this. It is a big help ❤️ !

klass.new(@_exposures.merge(object: obj, resource: resource))
end

if resources.nil? || Array(resources)[0].instance_variable_get(:@__graphiti_resource)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do resources = Array(resources) here so we don't have to do it further down the line?


if Mime[:jsonapi].nil? # rails 4
Mime::Type.register('application/vnd.api+json', :jsonapi)
if Mime[:graphiti].nil? # rails 4
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is OK to make this a configuration option, but not a default. The majority of our users do not run both gems at the same time, and I think :jsonapi makes a lot more sense for these users. Could we make this configurable?

@richmolj
Copy link
Contributor

richmolj commented Jan 7, 2019

@caseyprovost this library has moved to https://github.com/graphiti-api/graphiti - it's the same code, but would you mind recreating the PR there? Looking through the comments I think we're almost there and I'd love to get this in.

wadetandy pushed a commit to wadetandy/graphiti_archived that referenced this pull request Jan 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants