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

Wrap auto-completed list input values with square brackets if required #587

Open
KyleAMathews opened this issue Sep 2, 2017 · 10 comments
Open

Comments

@KyleAMathews
Copy link

In Gatsby's connections, you can add a sort input like the following:

allMarkdownRemark(sort: { fields: [frontmatter___title], order: DESC}) {
  edges {
    node {
      id
    }
  }
}

But in graphiql, it doesn't enforce that fields must be an array so people regularly complain sorting is broken as they just add a field in the UI and then when they try to copy that into a project it won't work.

Is there a reason graphiql is like this? It seems to be casting the single values to an array which is why it seems to work.

@KyleAMathews KyleAMathews changed the title Enforce list Enforce list inputs Sep 2, 2017
@coreyward
Copy link

+1 Currently Relay will cast a single value to a string; graphiql's deviation from this causes incompatibilities.

@OlegIlyenko
Copy link
Contributor

OlegIlyenko commented Jun 29, 2018

If I understand what you are describing correctly, then this aspect is covered by the GraphQL spec:

If the value passed as an input to a list type is not a list and not the null value, it should be coerced as though the input was a list of size one, where the value passed is the only item in the list. This is to allow inputs that accept a “var args” to declare their input type as a list; if only one argument is passed (a common case), the client can just pass that value rather than constructing the list.

In other words, it is ok to skip list syntax (e.g. [...]) if list has only one element.

@coreyward
Copy link

@KyleAMathews Is Gatsby doing something custom on top of Relay or is this a bug with Relay? I don't know enough to tell (couldn't find the intermediary files produced by Relay in my .cache).

@acao acao added the bug label Dec 2, 2019
@acao acao added this to Needs triage in Bug Triage via automation Dec 2, 2019
@acao
Copy link
Member

acao commented Dec 2, 2019

@coreyward so, gatsby is not using relay, they have their own graphql schema implementation on top of graphql-js.

what @OlegIlyenko's pointing out here is a valid bug incodemirror-graphql's implementation of the official GraphQL spec. and it's our job to adhere to the spec, so this is a bug that would impact relay's IDE implementation as well as many others.

@benjie
Copy link
Member

benjie commented Dec 10, 2019

I couldn't find an example of this in the SWAPI API, so I went to GitHub's API where they have the marketplaceCategories field which accepts a parameter called includeCategories which is a list of strings. https://developer.github.com/v4/explorer/

Issuing this query:

{
  ai: marketplaceCategories(includeCategories: "ai-assisted") {
    name
    slug
  }
  aiOrChat: marketplaceCategories(includeCategories: ["ai-assisted", "chat"]) {
    name
    slug
  }
}

against the GitHub API correctly returns this result:

{
  "data": {
    "ai": [
      {
        "name": "AI Assisted",
        "slug": "ai-assisted"
      }
    ],
    "aiOrChat": [
      {
        "name": "AI Assisted",
        "slug": "ai-assisted"
      },
      {
        "name": "Chat",
        "slug": "chat"
      }
    ]
  }
}

As @OlegIlyenko correctly points out the "Input Coercion" section of TypeSystem > List in the GraphQL spec allows for a single list item to be specified without using the brackets. GraphiQL seems to support this just fine, so I don't think we have a bug here.

That said, having the auto-complete automatically insert list wrappers around an option selected from a list type would be a welcome addition as sometimes people rely on auto-complete rather than the docs and do not realise that a list is possible in a particular position.

@benjie benjie removed the bug label Dec 10, 2019
@benjie benjie changed the title Enforce list inputs Wrap auto-completed list input values with square brackets if required Dec 10, 2019
@acao
Copy link
Member

acao commented Dec 10, 2019

i was able to reproduce the error the other day, its a bug

@acao acao added the bug label Dec 10, 2019
@benjie
Copy link
Member

benjie commented Dec 10, 2019

Can you expand on what the error/bug is, please?

@acao
Copy link
Member

acao commented May 3, 2020

given @OlegIlyenko 's point re: the spec, do we feel this is indeed a bug?

@coreyward do you feel this behavior is to spec? or is there a way we can support the spec and multiple expectations here?

@amanprateek123
Copy link

@acao I want to contribute in open source in graphql. How can I start?

@acao
Copy link
Member

acao commented Nov 26, 2021

this is a place where additionalInsertText is used. we would add it to graphql-language-service-interface in the completion response for getAutocompleteSuggestions, and then handle that information in graphql-language-service-interface when normalizing the completion data in the utility at the end of MessageProcessor.ts.

vscode-json-languageservice has an excellent implementation of this that we can learn from, though their needs for insertText might be more complicated than ours.

our most important insertText is this feature by far, the reference example shows how they do this for for json arrays and objects using json schema. I already can show how to filter completion items to return the insertText when and where we need, just need to finish normalizing the insertText information across the service layers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Bug Triage
  
Needs triage
Development

No branches or pull requests

6 participants