strapi / rfcs

RFCs for Strapi future changes
70 stars 33 forks source link

GRAPHQL API #29

Closed Convly closed 2 years ago

Convly commented 3 years ago

GraphQL API v4 This PR introduces a draft version of the GRAPHQL API in v4.

We want to get the conversation started around this topic to get feedbacks quickly so feel free to ask questions and give ideas :)

You can read it here

strapi-cla commented 3 years ago

CLA assistant check
All committers have signed the CLA.

derrickmehaffy commented 3 years ago

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/new-version-questions-thoughts-v4/4824/1

gu-stav commented 3 years ago

Thanks for posting this. I do have a couple remarks / questions:

{
  documents(publicationState: { $eq: "live" }) {}
}

This could also be solved, if the pagination wasn't done by an index, but by a hash, so the content of a page doesn't change.

{
  documents(
    sort: {
      fields: [title]
      order: ASC
    }
  ) {}
}

To support more than one dimension it could look like:

{
  documents(
    sort: {
      fields: [title, publidationDate]
      order: [ASC, DESC]
    }
  ) {}
}
Stun3R commented 3 years ago

You didn't talked about the schema.json. Will you make some change as it's autogenerated ? (Working on the SDK to generate types based on the schema.json you provide)

alexandrebodin commented 3 years ago

You didn't talked about the schema.json. Will you make some change as it's autogenerated ? (Working on the SDK to generate types based on the schema.json you provide)

We are going to continue export the schema but it will jsut have the new format :) Anything specific you had in mind ?

Stun3R commented 3 years ago

You didn't talked about the schema.json. Will you make some change as it's autogenerated ? (Working on the SDK to generate types based on the schema.json you provide)

We are going to continue export the schema but it will jsut have the new format :) Anything specific you had in mind ?

It would be great if you could separate the schema and the operations in 2 specific file: schema.json | schema.graphql & operations.json | operations.graphql or at least give better name to the interface & type created in the schema 🤔

SalahAdDin commented 3 years ago

Do you have any plan to support Subscriptions?

DigitalCop commented 3 years ago

Do you plan to support behind a firewall/proxy support? Current version does not work due to a CDN request to an external resource. Found a workaround, but still...

alexandrebodin commented 2 years ago

Linking an issue discussing the Graphql API and some pain points.

Convly commented 2 years ago

Do you have any plan to support Subscriptions?

Little update, a first version of the subscriptions has been implemented in https://github.com/strapi/strapi/pull/11129 and is available in the latest beta

alexandrebodin commented 2 years ago

Copy pasting and answering from the issue here :) Issues: https://github.com/strapi/strapi/issues/11649

Current pain points(s)

  1. Defining routes is a pain, especially if compared to what it was like in V3

I'm not sure I understand. What do you mean by routes ? queries & mutations ? resolvers ?

  1. Managing User Permissions is much harder and convoluted (somewhat related to the first pain point)

Can you explain with a given example ?

  1. While extending Mutation the way it currently is needed to is not per se Strapi related problem, it would be amazing if Strapi would handle this by simply creating a new resolver (I believe this is somewhat how it worked in V3)

I'm lost on this one what do you mean ?

  1. The response format is not great for those working with say Apollo Client 3 (or other clients with cache normalization), where the GQL Client relies on a unique identifier inside it GQL type to track the given object. So having the ID inside the attributes would be beneficial. Furthermore the nested data {} inside the response it somewhat verbose and not sure if it's great DX, not only for the sake of the proportionally increasing nullity checks with each nested relation, but also since it - again - increases the complexity for using the GQL Client with cache normalization (this could be solved with something like custom merge functions, but that surely would not be a good DX. Perhaps this could be solved by moving the metadata inside the actual response, say instead of
query {
  posts {
    data {
      id
      attributes {
        title
        ...
      }
    }
    meta {
      pagination {
        ...
      }
    }
  }
}

it could be this:

query {
  posts {
    id
    title
    meta {
      pagination
      ...
    }
  }
}

this format would add the pagination meta for each entry here I don't think this is what you wanted to do ?

ScottAgirs commented 2 years ago

@alexandrebodin thanks for reposting, very keen on helping in any way I can. Again - sadly, this is a major roadblock for me to switch from V3 to V4, for now, I had to shelf the migration to V4 and keep working with V3.

krstivojevicv commented 2 years ago

@alexandrebodin Regarding the point 4:

This seems really nice:

query {
  posts {
    id
    title
    meta {
      pagination
      ...
    }
  }
}

If you are not comfortable with meta being inside attributes scope you can use this approach (this is how it works in Directus)

query {
  posts {
    data {
      id
      title
    }
    meta {
      pagination
    }
  }
}

But the first approach seems more convenient and having meta for each entry can actually be an advantage

ScottAgirs commented 2 years ago

@alexandrebodin Regarding the point 4:

This seems really nice:

[..]

If you are not comfortable with meta being inside attributes scope you can use this approach (this is how it works in Directus)

query {
  posts {
    data {
      id
      title
    }
    meta {
      pagination
    }
  }
}

[..]

This format would likely still cause headaches because it involves extra layer of nesting and also the actual collection type does not have an identifier on its root level.

However, the first option where meta {} is part of the root collectionType {} The meta {} could in turn have its own ID attached to it. That setup would likely make working with it on a client a breeze.

abdonrd commented 2 years ago

I'm also more in favor of the flat format.

valdeniomarinho commented 2 years ago

Sadly I still prefer the V3. My opinion is that V4 is way too much verbose and not good DX.

V4 meta{}, data{}, attributes{} : YAGNI and DRY V3 flat and simple : KISS

derrickmehaffy commented 2 years ago

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/discussion-regarding-the-complex-response-structure-for-rest-graphql-developer-experience/13400/1

tilman commented 2 years ago

Sadly I still prefer the V3. My opinion is that V4 is way too much verbose and not good DX.

meta{}, data{}, attributes{} : YAGNI and DRY flat and simple : KISS

Totally agree to this! Especially if we access deeply nested collections via relations the code get's very long and unreadable.

abdonrd commented 2 years ago

I think that this new structure makes everything much more complex.

With this level of nesting…

I would love to see this structure simplified.

Also this is a plugin, I think a new v5 version could even be created with breaking changes while Strapi stays at v4.

santiagok986 commented 2 years ago

I love V3, simple, easy and quick to use

query($id:ID!){
  recipe(id:$id){
    id
    name
    img
    ingredients
    steps
    category{
      id
      name
      slug
    }
    autor{
      username
    }
  }
}

V4 :( more complex and useless nested

query($id:ID!){
  recipe(id:$id){
    data{
      id
      attributes{
        name
        img
        ingredients
        steps
        category{
          data{
            id
            attributes{
              name
              slug
            }
          }
        }
         autor{
          data{
            attributes{
              username
            }
          }
        }
      }
    }
  }
}

V3 vs V4

Use V3 :) => recipe.name Use V3 :) => recipe.category.name

Use V4 :/ => recipe.data.attributes.name Use V4 :( => recipe.data.attributes.category.data.attributes.name

Sepifanovskiy commented 2 years ago

New response structure results in unreadable code when working with nested collection. But event shallow responses make code ugly with all unnecessary wrappers.

IsaacDdR commented 2 years ago

One of the main purposes of GraphQL is lost with this over-engineered "solution"

Dorifor commented 2 years ago

I also think that the new API response is bloated and much harder to read / process in our apps. It's now harder to make it works and easier to make mistakes (and lose time / sanity)

It's a bit of a shame honestly, hoping to see it take a better direction in the future

SalahAdDin commented 2 years ago

Is there any way to make out the v4 response version and just use the v3?

Thanks

nergmada commented 2 years ago

Hi, been using Strapi v3 in a number of projects. The changes in v4 feel like change for the sake of change. Love the new interface etc. but not enough to want to hack together AWS-bucket plugins for media upload to digitalocean etc. (but I digress as this isn't the main comment I want to add).

This change seems arbitrary, unnecessary, and lacking an intuitive feel. I don't tend to read docs before I start firing REST requests or GraphQL at a project, so when I fired my first request at a v4 project, I was already weirded out by the response to a query like below.

query {
  blog {
    title
    body
  }
}

To then find out I had to go two layers deeper to not just data but attributes, this seemed excessive. I personally agree with the sentiment of a flat response structure (as query above shows), but I can see the merit of metadata being kept separate as below

query {
  blog {
    title
    body
    meta {
      published_at
    }
  }
}

but expressing the main content as a sub-object of a sub-object within the response doesn't make any sense because surely the reason we're sending a request is for that data. Admittedly sometimes we only want metadata, but in most cases, I suspect we want something like a title, or a link, or a description from the main content.

I particularly agree with the sentiment about null checks, which I think in both the scenarios above are the same. You technically don't need to do null checks on metadata if content data is there because content data probably wouldn't be sent without metadata (he says brazenly).

Jacob87 commented 2 years ago
rowData?.attributes?.service?.data?.attributes?.price?.data?.attributes?.author?.data?.attributes?.name

😭😭

pechitook commented 2 years ago

Adding a comment to emphasize something I didn't see anyone else raise: the nesting issue becomes unnecessarily verbose when you're handling media files that are declared as single-file types. Here's a real example from our migration attempt:

V3

query HeaderData {
  siteContent {
    id
    logo {
      url
    }
  }
}

V4

query HeaderData {
  siteContent {
    data {
      id
      attributes {
        logo {
          data {
            attributes {
              url
            }
          }
        }
      }
    }
  }
}

I agree this can come up a bit counter intuitive for graphql folks who are familiar with the flexibility Apollo Client provides. This seems inspired on the Relay pagination strategy, which of course works but what I loved about Strapi is that it was simple by default and you could always extend it as a regular NodeJS server if you needed to.

Keep up the good work!

stafyniaksacha commented 2 years ago
rowData?.attributes?.service?.data?.attributes?.price?.data?.attributes?.author?.data?.attributes?.name

😭😭

This is a pain to handle in typed languages (like in dart, typescript, rust, go). Is there any new on this ?

edit: found on the forum:

At this time we are not prepared or planning to modify this data structure in the response, the time for that type of modification has passed as it should have been addressed during the RFC process. We do understand that it was not clear at the time during our RFC process this was the case and we are already doing retrospectives to improve this process.

SalahAdDin commented 2 years ago

Adding a comment to emphasize something I didn't see anyone else raise: the nesting issue becomes unnecessarily verbose when you're handling media files that are declared as single-file types. Here's a real example from our migration attempt:

V3

query HeaderData {
  siteContent {
    id
    logo {
      url
    }
  }
}

V4

query HeaderData {
  siteContent {
    data {
      id
      attributes {
        logo {
          data {
            attributes {
              url
            }
          }
        }
      }
    }
  }
}

I agree this can come up a bit counter intuitive for graphql folks who are familiar with the flexibility Apollo Client provides. This seems inspired on the Relay pagination strategy, which of course works but what I loved about Strapi is that it was simple by default and you could always extend it as a regular NodeJS server if you needed to.

Keep up the good work!

Oh God, my eyes, my EYES!!!

onurbamaro commented 2 years ago

yeah, I was just setting things up to upgrade my project to V4 until I first came accross the first query with GraphQL.

Dropped the idea of upgrading it until its more like V3.

cumironin commented 2 years ago

Rest in V4 is very painfull to use, so much easier in V3

strapi-bot commented 2 years ago

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/how-to-use-graphql-with-strapi4/16260/2

strapi-bot commented 2 years ago

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/how-to-use-graphql-with-strapi4/16260/4

matthill8286 commented 2 years ago

I have used V4 for a variety of projects and couldn't understand why everything in the GQL land is ....data.attributes, it's clunky, so I have reverted back to use V3 as the flat structure also follows the graphql spec https://spec.graphql.org/October2021/, i'm also not sure extracting the ID! outside attributes will play well with Apollo caching when you have nested queries, but I guess time will tell.

Whats the reason as so far I can find one, unless i've missed as to why the need to change the GQL offering??

SalahAdDin commented 2 years ago

Does anyone try to use this plugin?

Does anyone on the Strapi Staff aware of these problems?

matthill8286 commented 2 years ago

Use this plug-in https://market.strapi.io/plugins/strapi-plugin-transformer, you can use V4 but with the decent V3 graphql response format. None of this crap they are offering in V4.

On Fri, 1 Apr 2022, 22:35 Isaac Díaz, @.***> wrote:

It's impressive how Strapi won't do anything about this. We're talking to a wall. I trusted Strapi and now I'm f***ed bc of this update.

— Reply to this email directly, view it on GitHub https://github.com/strapi/rfcs/pull/29#issuecomment-1086258428, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALQSRH2SPSOIJG6SVXAXJKTVC5FXTANCNFSM45M4X5XA . You are receiving this because you commented.Message ID: @.***>

santiagok986 commented 2 years ago

The plugin https://market.strapi.io/plugins/strapi-plugin-transformer, only works in REST, how can I make it work in GraphQL ?

SalahAdDin commented 2 years ago

The plugin https://market.strapi.io/plugins/strapi-plugin-transformer, only works in REST, how can I make it work in GraphQL ?

Really? No way!

derrickmehaffy commented 2 years ago

@SalahAdDin

Does anyone on the Strapi Staff aware of these problems?

Yes we are aware but functionally we can't change this in v4 as it would be a breaking change and the structure specified is needed for some of our upcoming features (primarily content-versioning)

SalahAdDin commented 2 years ago

@SalahAdDin

Does anyone on the Strapi Staff aware of these problems?

Yes we are aware but functionally we can't change this in v4 as it would be a breaking change and the structure specified is needed for some of our upcoming features (primarily content-versioning)

Is there any other solution for this? Cause this structure is really horrible, we had to keep the 3 version cause the response format.

SalahAdDin commented 2 years ago

@SalahAdDin

Does anyone on the Strapi Staff aware of these problems?

Yes we are aware but functionally we can't change this in v4 as it would be a breaking change and the structure specified is needed for some of our upcoming features (primarily content-versioning)

As @alexandrebodin closed this issue, should we understand it will be a solution soon? or won't it be fixed never?

alexandrebodin commented 2 years ago

Hello @SalahAdDin,

We closed this RFC as this was already implemented and is not the place for issue management.

Please do comment in the forum thread if you want to add any suggestions there. We did share the reasons we need this format in the mid and long term there, this might give you the information you are looking for. If you find some solutions that we could use while addressing our constraints we would be glad to hear about them!