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

Support for lists with nullable elements #119

Closed
erencay opened this issue Jul 28, 2018 · 15 comments
Closed

Support for lists with nullable elements #119

erencay opened this issue Jul 28, 2018 · 15 comments
Labels
Bug 🐛 Something isn't working Community 👨‍👧 Something initiated by a community Solved ✔️ The issue has been solved
Milestone

Comments

@erencay
Copy link

erencay commented Jul 28, 2018

AFAIK there is currently no support for lists with nullable elements, such as [String]!.

export function wrapWithTypeOptions<T extends GraphQLType>(
  type: T,
  typeOptions: TypeOptions = {},
): T {
  let gqlType: GraphQLType = type;
  if (typeOptions.array) {
    gqlType = new GraphQLList(new GraphQLNonNull(gqlType));
  }
  if (!typeOptions.nullable) {
    gqlType = new GraphQLNonNull(gqlType);
  }
  return gqlType as T;
}

Seems like this method forces all lists to have non-null elements.

Appopriate solutions would be either adding a null value to value array
@Field(type => [String, null])

Or providing a decorator option such as;
@Field(type => [String], { nullable: true, nullableElement: true })

@19majkel94 would you like me to make a pull request?

@MichalLytek
Copy link
Owner

AFAIK there is currently no support for lists with nullable elements, such as [String]!.

Yes, this is by design. A thought about this for a while and I couldn't figure out a legit use case for returning array of e.g. integers mixed with empty null values. Nullable array makes sense (auth guards, etc.) bo not a sparse array. Could you describe your use case when your array contains nulls?

nullableElement looks awful, I would rather change type of nullable to boolean | "itemsOnly" | "itemsAndArray" or sth like this.

@MichalLytek MichalLytek added Enhancement 🆕 New feature or request Community 👨‍👧 Something initiated by a community Discussion 💬 Brainstorm about the idea Need More Info 🤷‍♂️ Further information is requested labels Jul 29, 2018
@erencay
Copy link
Author

erencay commented Jul 29, 2018

In my case I'm building a game and I have a fixed length array which filled with null values by default. As the game progress I need to mutate values at certain indexes. So I think one might need this feature where there is a fixed length of array filled with null values, index matters and array contains non-scalar type.

Something like:

[null, null, null, null, null, null, null, null, null]
[null, null, null, null, null, null, { number: 1, color: "red" }, null, null]

There are workarounds but this structure feels most natural.

@MichalLytek
Copy link
Owner

MichalLytek commented Jul 29, 2018

I think that your app should abstract your internal, array representation from the public API.

In GraphQL there's no array, only lists (GraphQLList as you see above), which means you cant access random element of the list.

So you should use the fieldResolver pattern to transform an array to list of entries:

type GameArrayItem {
  index: Int!
  value: GameItemValue!
}

type GameItemValue {
  number: Int!
  color: String!
}

So you query should return list of GameArrayItem:

type Query {
  items: [GameArrayItem!]!
  item(index: Int!): GameArrayItem
}

query GetItems {
  items {
    index
    value {
      number
      color
    }
  }
}

query GetItem1 {
  item(index: 1) {
    value {
      number
      color
    }
}

And mutation should operate on index:

type Mutation {
  gameProgress(index: Int!, value: GameProgressInput): Boolean
}

input GameProgressInput {
  number: Int!
  color: String!
}

mutation ProgressGame {
  gameProgress(index: 1, value: {
	number: 55
    color: "blue"
  })
}

Not on the whole list replacing items by itself.

@erencay
Copy link
Author

erencay commented Jul 29, 2018

You are right. This makes sense. Normally I would like to avoid such abstractions but I think this is how I should handle things in Graphql.

@MichalLytek MichalLytek removed the Enhancement 🆕 New feature or request label Jul 29, 2018
@erencay
Copy link
Author

erencay commented Jul 29, 2018

@19majkel94 On a side note I still think type-graphql should support this feature for compatibility/comprehensiveness sake.

@MichalLytek MichalLytek added Bug 🐛 Something isn't working and removed Discussion 💬 Brainstorm about the idea Need More Info 🤷‍♂️ Further information is requested labels Aug 5, 2018
@MichalLytek
Copy link
Owner

@erencay You're right, I should keep 100% compatibility with GraphQL specs.
But nullable list elements are so rare that you are the first one that have reported this inconvenience from the beginning of the project 5 months ago.

So this one will be done but with the lowest priority. And I have to find a good API for this case 😉

@lukejagodzinski
Copy link

Actually when working on one of the projects I needed such a feature. Let's imagine that your database has some holes, some references that does not point to any objects. For example in project for our client we had something like this (it's not real example but just illustration):

  • it was MongoDB database
  • there was the orders collection
  • there was the users collection
  • each order had the userIds array field where there were references to users where some of them were missing

and we had to implement GraphQL endpoint for such a data. So we had schema:

type Order {
  id: ID!
  users: [User]!
}

PS. It's not a real example but I just wanted to present that this case might happen

@MichalLytek
Copy link
Owner

MichalLytek commented Oct 12, 2018

It's not a real example but I just wanted to present that this case might happen

That's the point. I haven't found good enough real example too 😃

If you really need it - feel free to open a PR 😉
The nullable option should accept:

  • true for [T!]
  • false for [T!]!
  • item for [T]!
  • and maybe itemAndList for [T]

@MichalLytek MichalLytek added Help Wanted 🆘 Extra attention is needed Good First Issue 👋 You can start contributing here Hacktoberfest labels Oct 12, 2018
@kenpusney
Copy link
Contributor

any progress on this issue? we have same problem here to generate "nullable element" array in graphql schema.

@MichalLytek
Copy link
Owner

@kenpusney
Feel free to make a PR or show me a real use case for sparse array 😉

@kenpusney
Copy link
Contributor

@19majkel94 please review. #211

Thanks.

@MichalLytek
Copy link
Owner

Closing this via #211 🔒

@MichalLytek MichalLytek added Solved ✔️ The issue has been solved and removed Good First Issue 👋 You can start contributing here Help Wanted 🆘 Extra attention is needed labels Dec 22, 2018
@aounleonardo
Copy link

I'm having this issue with resolver arguments:

@FieldResolver((returns) => [Message], { nullable: true })
    async messages(
        @Root() user: User,
        @Ctx("viewer") viewer: ViewerContext,
        @Arg("range", (type) => [Date], { nullable: "itemsAndList", defaultValue: [null, null] }) range: [Date, Date],
    )

Let's say I want to ask for a certain range of messages, but I could ask for something like: [null, new Date()] which means anything until now. I'm getting an Argument Validation Error, I also tried with [Int] and [String] so Date is not the problem.

It's not a pressing issue, but do you know if GraphQL even allow this type of arguments?

@MichalLytek
Copy link
Owner

@aounleonardo Argument Validation Error comes from validation of args using class-validator and as your code is not using them, I think the issue is in a different level of your query

@aounleonardo
Copy link

So the {nullable: "itemsAndList"} doesn't have any effect on what class-validator does? My bad then.. If I turn validate to false in buildSchema it works 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 Something isn't working Community 👨‍👧 Something initiated by a community Solved ✔️ The issue has been solved
Projects
None yet
Development

No branches or pull requests

5 participants