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

[v4] set GraphQL-Entity props required #11815

Open
sebastian-ludwig opened this issue Dec 5, 2021 · 21 comments · May be fixed by #20419
Open

[v4] set GraphQL-Entity props required #11815

sebastian-ludwig opened this issue Dec 5, 2021 · 21 comments · May be fixed by #20419
Labels
issue: bug Issue reporting a bug severity: low If the issue only affects a very niche base of users and an easily implemented workaround can solve source: plugin:graphql Source is plugin/graphql package status: confirmed Confirmed by a Strapi Team member or multiple community members

Comments

@sebastian-ludwig
Copy link

sebastian-ludwig commented Dec 5, 2021

Bug report

In your GraphQL-Example is this example:

type DocumentEntity {
  id: ID
  attributes: Document
}
​
type DocumentEntityResponse {
  data: DocumentEntity
}
​
type DocumentEntityResponseCollection {
  data: [DocumentEntity!]!
  meta: ResponseCollectionMeta!
}

ID and attributes shouldn't be optional in documentEntity, if there is a valid DocumentEntity data.
Background: I create TypeScript-Classes for the Frontend based on the schema. In v3 I could just check if the data has arrived and then work with the data, in v4 I would need to check the data and then if id and attributes exists, that does not seem to be necessary.

Expected behavior

type DocumentEntity {
  id: ID!
  attributes: Document!
}

System

  • Node.js version: v14.18.0
  • NPM version: 6.14.15
  • Strapi version: 4.0.0
  • Database: PostgreSQL
  • Operating system: MacOS

Solution

// @strapi/plugin-graphql/server/services/builders/entity.js
t.nonNull.id('id', { resolve: prop('id') });
t.field('attributes', {
  type: nonNull(typeName),
  resolve: identity,
});
@derrickmehaffy
Copy link
Member

Hello,

I see you are wanting to ask a question that is not really a bug report or a feature request, questions should be directed to our forum. Please see the following contributing guidelines for asking a question here.

@sebastian-ludwig sebastian-ludwig changed the title [v4] se GraphQL-Entity props required? [v4] set GraphQL-Entity props required Dec 6, 2021
@sebastian-ludwig
Copy link
Author

@derrickmehaffy Thank you, I should not have formulated it as a question.
After taking another look into the Code, I consider this as a bug, so I updated my description, hope this is ok?
Could you reopen the issue please?

@derrickmehaffy
Copy link
Member

@derrickmehaffy Thank you, I should not have formulated it as a question. After taking another look into the Code, I consider this as a bug, so I updated my description, hope this is ok? Could you reopen the issue please?

You are missing the system block of the template, if you could please add that and respond back and I can reopen.

@sebastian-ludwig
Copy link
Author

@derrickmehaffy Here we go

@derrickmehaffy derrickmehaffy reopened this Dec 9, 2021
@derrickmehaffy derrickmehaffy added severity: low If the issue only affects a very niche base of users and an easily implemented workaround can solve source: plugin:graphql Source is plugin/graphql package Squad: Developer Experience status: pending reproduction Waiting for free time to reproduce the issue, or more information issue: bug Issue reporting a bug labels Dec 9, 2021
@derrickmehaffy derrickmehaffy added this to To be reviewed (Open) in Developer Experience - Old via automation Dec 9, 2021
@derrickmehaffy derrickmehaffy added status: confirmed Confirmed by a Strapi Team member or multiple community members and removed status: pending reproduction Waiting for free time to reproduce the issue, or more information labels Jan 5, 2022
@christiaan-lombard
Copy link

This issue makes for a very bad experience when building a TS client application. Types generated from Strapi's GraphQL schema often contains null and undefined that requires mapping on the client side.

eg. Dynamic Zone

type Page {
  title: String!
  body: [PageBodyDynamicZone]!
}

maps to

export type Page = { 
  __typename: 'Page', 
  title: string,
  body: Array<PageBodyDynamicZone | null | undefined> // ?
}

eg. Query

query Pages {
  pages {
    data {
      id
      attributes {
        ...PageAttributes
      }
    }
  }
}

maps to

export type PagesQuery = {
  __typename: 'Query',
  pages?: null | undefined | { // ?
    __typename: 'PageEntityResponseCollection',
    data: Array<{
      __typename: 'PageEntity',
      id?: null | undefined | string, // ?
      attributes?: null | undefined | PageAttributes // ?
    }>
  }
};

Related issue: 9411

These types require copious amounts of useless null/undefined checks on the client side, not to mention mapping returned arrays with:

export function mapDefiniteArray<T>(source: Array<T | undefined | null>): Array<T> {
  return source as Array<T>;
}

Why are these GQL type definitions not required?

@dottodot
Copy link

Yes still definitely isn't right, the ID shouldn't ever be null or undefined.

@frankadrian
Copy link

Hello, I just experienced the same problem, type generated for Typescript contains null even though the ID for example should never be nullable.
When setting a custom field to required via the Strapi admin the field also shows up as not nullable in the generated types. So this works as expected, but IDs should never be nullable. Please correct me if I'm wrong.

Here is an example of a Collection type using the graphql api extension for autogenerated endpoints:

export interface Parts_parts_data {
  __typename: "PartEntity";
  id: string | null; // <- id should not be nullable
  attributes: Parts_parts_data_attributes | null; // <- also here attributes should at least be an object of the said type
}

export interface Parts_parts {
  __typename: "PartEntityResponseCollection";
  data: Parts_parts_data[];
}

export interface Parts {
  parts: Parts_parts | null; // <- also should not be nullable. 
}

I would love to create an MR for this, is it something you would consider?

Kind regards

@itzaks
Copy link

itzaks commented Dec 6, 2022

Hi, I'm experiencing the same. I really enjoy strapi, but these properties being marked as nullable makes my typescript experience quite messy. Would love to see a fix on this.

@axelinternet
Copy link

Anyone have a workaround for this? Working with generated types from the graphql is such a mess.

Have you made workarounds in your scripts (to not follow the schema...) or something else?

@cesar3030
Copy link

Struggling with the same issue here. Even __typename is optional for me which is impossible.

Screen Shot 2023-03-22 at 20 13 41

@ehsmsh
Copy link

ehsmsh commented Mar 30, 2023

It seems to be a headache working with TS client like this. Any updates or workarounds?

@axelinternet
Copy link

How are you guys handing this in your applications? Huge headache for me

@rikkit
Copy link

rikkit commented Aug 23, 2023

Lots and lots of of ?....

@halavasty
Copy link

I had to abandon Strapi because of this behavior 🥹
so sorry

@christiaan-lombard
Copy link

We use mapping functions to get rid of these crappy types.

Here is a few examples:

export function mapEntity<A>(data: { id?: string | null; attributes?: A | null }): {
  id: string;
  attributes: A;
} {
  return data as { id: string; attributes: A };
}

export function mapDefiniteArray<T>(source: Array<T | undefined | null>): Array<T> {
  return source as Array<T>;
}

😒

@michalhonc
Copy link

michalhonc commented Nov 24, 2023

We are patching the documentation plugin to make ID and attributes required

Patch file location (mind the lib version): patches/@strapi+plugin-documentation+4.15.2.patch
Patch file content:

diff --git a/node_modules/@strapi/plugin-documentation/server/services/helpers/build-component-schema.js b/node_modules/@strapi/plugin-documentation/server/services/helpers/build-component-schema.js
 index 95fb824..54486c1 100644
 --- a/node_modules/@strapi/plugin-documentation/server/services/helpers/build-component-schema.js
 +++ b/node_modules/@strapi/plugin-documentation/server/services/helpers/build-component-schema.js
 @@ -222,6 +222,7 @@ const getAllSchemasForContentType = ({ routeInfo, attributes, uniqueName }) => {
        }),
      },
      [`${pascalCase(uniqueName)}ResponseDataObject`]: {
 +      required: ['id', 'attributes'],
        type: 'object',
        properties: {
          id: { type: 'number' },

and run (setup patch-package first)

npx patch-package @strapi/plugin-documentation

this will then generate those fields as required (tested for OpenAPI spec). Not a bulletproof solution so be cautious

@smonkey72
Copy link

I'm amazed that the ticket hasn't budged in 2 years. Working with Strapi/GraphQL in TypeScript projects feels almost impossible. 🤔

@momesana
Copy link

I am also struggling with this. Any feedback from the Strapi developers?

@momesana
Copy link

momesana commented May 24, 2024

This is an immensely annoying bug. It's weird that it has been assigned a severity of 'low'. Lots of people use typescript these days and this bug makes it next to impossible to use typescript with the generated graphql types unless one litters the codebase with useless type assertions which are only there to make up for a bug that should've been addressed by the strapi team 2 years ago when the issue was first reported. Also none of the workarounds for graphql work anymore as the code has been refactored and shifted around beyond recognition since they were first described.

msadegh76 added a commit to msadegh76/strapi that referenced this issue Jun 1, 2024
@msadegh76 msadegh76 linked a pull request Jun 1, 2024 that will close this issue
@momesana
Copy link

momesana commented Jun 11, 2024

Any chance one of the Strapi maintainers (@derrickmehaffy maybe) may look at the PR put forward by @msadegh76 and approve it or give suggestions on how to improve it? The diff looks simple enough that a review could probably be done without too much effort.

@LufyCZ
Copy link

LufyCZ commented Jun 19, 2024

Here's a patch pulled from the PR:

patches/@[email protected]

diff --git a/dist/server/index.js b/dist/server/index.js
index 5221af3c4a7c9bbb4aa3f201aa98b46fff29a5e8..04631eeb2de923fb121e76a3ba3898e1f90bdea1 100644
--- a/dist/server/index.js
+++ b/dist/server/index.js
@@ -1433,9 +1433,9 @@ const entity = ({ strapi: strapi2 }) => {
       return nexus.objectType({
         name,
         definition(t) {
-          t.id("id", { resolve: fp.prop("id") });
+          t.nonNull.id("id", { resolve: fp.prop("id") });
           if (!fp.isEmpty(attributes2)) {
-            t.field("attributes", {
+            t.nonNull.field("attributes", {
               type: typeName,
               resolve: fp.identity
             });
@@ -1780,7 +1780,7 @@ const createCollectionTypeQueriesBuilder = ({ strapi: strapi2 }) => {
     const { uid } = contentType2;
     const findQueryName = getFindQueryName(contentType2);
     const responseCollectionTypeName = getEntityResponseCollectionName(contentType2);
-    t.field(findQueryName, {
+    t.nonNull.field(findQueryName, {
       type: responseCollectionTypeName,
       args: getContentTypeArgs(contentType2),
       async resolve(parent, args2, ctx) {
diff --git a/dist/server/index.mjs b/dist/server/index.mjs
index 6fba041a442521c3e645a5034cc65fe785eb4ecf..a8c488e60924851160a90793e14bcc0be447b101 100644
--- a/dist/server/index.mjs
+++ b/dist/server/index.mjs
@@ -1412,9 +1412,9 @@ const entity = ({ strapi: strapi2 }) => {
       return objectType({
         name,
         definition(t) {
-          t.id("id", { resolve: prop("id") });
+          t.nonNull.id("id", { resolve: prop("id") });
           if (!isEmpty(attributes2)) {
-            t.field("attributes", {
+            t.nonNull.field("attributes", {
               type: typeName,
               resolve: identity
             });
@@ -1759,7 +1759,7 @@ const createCollectionTypeQueriesBuilder = ({ strapi: strapi2 }) => {
     const { uid } = contentType2;
     const findQueryName = getFindQueryName(contentType2);
     const responseCollectionTypeName = getEntityResponseCollectionName(contentType2);
-    t.field(findQueryName, {
+    t.nonNull.field(findQueryName, {
       type: responseCollectionTypeName,
       args: getContentTypeArgs(contentType2),
       async resolve(parent, args2, ctx) {

(btw it's crazy that this is somehow still open)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug Issue reporting a bug severity: low If the issue only affects a very niche base of users and an easily implemented workaround can solve source: plugin:graphql Source is plugin/graphql package status: confirmed Confirmed by a Strapi Team member or multiple community members
Projects
Status: To be reviewed (Open)
Status: Reproducible on v4
Development

Successfully merging a pull request may close this issue.