-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Feature request: JSON5 Variables #780
Comments
The "Query Variables" input at the moment is JSON, so comments are not supported. One option would be to use JSON5 instead of JSON for the variables, and we could convert it to plain old JSON before we send it to the fetcher. This seems like a great candidate for a v1 feature; what do you think @acao? |
I agree, JSON5 support would be handy and would guarantee backwards compatibility. |
It sounds cool, but I think the multipart stuff would be a whole can of worms 😩whereas JSON5 could be implemented cleanly in a few hours with a seamless upgrade path.👌 |
@corysimmons agreed, just talking about how this will be pluggable in the future, but the default plugin would be JSON5 for v1, which is still a ways out |
sadly, the default will probably not be json5 necessarily, at least not right away. the reason being is that I am unsure we would be able to parse that in monaco, but we might. is tsconfig style json ok? it allows comments but still requires double quotes, and i think it doesnt care about trailing commas? that's easier to achieve with monaco. it would accomplish what you're asking for, very similar to, say, commenting out some compilerOptions in tsconfig. I can also see the need for a full-blown JSON 5 or HJSON style parsing. another reason I'll advocate for this: when i've used GraphiQL in the past, I often find myself pasting variables from unit tests, snippets, seed data, etc into the variables panel, and to do so you often need a js -> json conversion utility when you get into complex variables. Not to mention it's belaborous to convert with even simple variables. another way this could work would be to transform the code on paste, or have a "paste as" shortcut/palette command |
We call that JSONC - https://komkom.github.io Which the default monaco JSON stuff should support out of the box (though you might have to change the language to |
@orta that's fabulous, I can add that to the GraphiQL Monaco PR now! |
I'm a little new to the GraphiQL code based. Is there a way to run this today? On a Monaco fork or something? |
This would be super handy to have as I'm running into an issue where we have documentation which opens in GraphiQL however it would be great to retain the comments in the variables |
@tadhglewis check out the JSONC support in the JSON comments, trailing commas, etc. the same parser used for tsconfig.json and others. not the full
then I just it's coming in [email protected] :) |
@jonathanawesome would like to see if you have some ideas about toggling formats. JSON5, yaml, forms generated from the JSON schema we already have, there are many options. this would be great for the prototype and, though we had it slated for graphiql@2 with monaco, it can wait til graphiql@3 since that is when monaco is being shipped |
@acao circling back here after getting into some prototype design work over the weekend. Does this need to be a run time toggle, or could it be an optional prop that's passed into the GraphiQL component? |
@jonathanawesome IMO both - we should find a way to implement it with the nascent plugin API, though from a UX perspective it could (should?) be presented as a toggle for sure! a few reasons:
the variable editor plugins should:
<Graphiql
plugins={[
pluginType: 'variables',
label: 'JSON5',
editorMode: () => import('codemirror-json5'),
compiler: async ()=> {
const { parse, serialize } = await import('json5')
// method names based on native JSON.<> methods.
// not every JSON alternate parser follows this, so just in case
return { parse, serialize }
}
]}
</GraphiQL> the also, if you're wondering how we will type the expanding plugin definition interfaces, i think just overloads with a static one caveat of this approach is that you need to specify |
I believe that using JSON5 would also parse all valid JSONC and JSON documents. There may be outliers with really unlikely unicode sequences being treated slightly differently, but for 99.99% of use cases I think JSON5 should cover all our needs. And with JSON5 the need for YAML is diminished IMO - the number of additional symbols you have to type is minimal. |
@benjie but it could lead to JSON5 that is not valid JSONC, breaking copy paste? |
Good point 👍 The person writing the code in would be making it invalid though - so that's kind of their choice. Even after copy/pasting their invalidly entered content, if they use something like prettier in their editor then when they save the file it'll auto-fix it back to JSONC anyway in most cases. We'd just have to make sure that any auto-completes that GraphiQL's language services add are double-quoted (i.e. JSON-compatible) and it should work for all three 🤞 In an ideal world we'd have this all be super pluggable, but honestly I think just using JSON5 for everyone would be a great first step and would save a significant amount of development effort (which can always be added later). |
agreed! JSON5 has become the ubiquitous standard, I say we go with it and yaml variables as has been requested as well! |
YAML 👎👎 YAML could introduce huge problems, not just from the complex and subtle syntax (are you looking forward to all the support requests where it turns out it’s just the user added one too many or too few spaces, or missed a dash, or whatever? Or users confused by the many and varied ways of entering strings, each with their own quirks?), but also features that YAML supports that JSON doesn’t, for example circular references. I don’t think YAML is a good fit for GraphQL, and would confuse more people than it would help. |
@benjie the monaco-yaml mode already accepts the same json schema API for validation, completion, etc that we are using for monaco, so it's quite easy for us to support. any issues as such are not ours, but are issues for monaco-yaml project. This is just about interoperability IMO. There are tools out there that use yaml for graphql variables already, for example the developers of an ML tool that requested this feature two years ago. If there is invalid yaml for the supplies variables schema, it will automatically show validation errors. I will build a demo to show you! |
Integrated validation and formatting of the YAML should help somewhat, but giving users validation errors doesn't necessarily stop them reaching out for help because they don't understand why the validation error is happening. For example, consider filtering participants by a list of origin countries: countries:
- US
- GB
- NO This would be parsed as If YAML is supported, it should be opt-in and not part of the default experience in my opinion. But this is merely my opinion, and you and the other GraphiQL maintainers should do what you think is best for the community and the project. |
@benjie I agree that it should be opt in, and not part of the default experience! the formatting is built into monaco-yaml as well. the goal is to make it so that users can provide a plugin for the variables editing pane so that we can also support forms, etc, whatever third party plugins users want to contribute. |
Here is the closed issue - there are a few options for microsoft/monaco-editor#2830 (comment) we need to open a new feature request, and I would be happy to help a contributor chase this down. |
I decided to write up a blog post about the many avenues to accomplishing this, assuming we extend the graphiql plugin API to allow custom variables editors @imolorhe and I are already underway on one of the routes! https://rikki.dev/posts/building-json5-mode-for-graphql-variables update: we've created a |
I find myself testing various queries a lot, and when I do this, it'd be really nice if I could just comment out some query variables (preferably with
cmd + /
shortcut) instead of having to delete them completely and then re-enter them when I want to test them out again.It's not a big deal, but it'd be nice.
The text was updated successfully, but these errors were encountered: