search

graphql / graphiql

GraphiQL & the GraphQL LSP Reference Ecosystem for building browser & IDE tools.
MIT License
16.1k stars 1.72k forks source link

Make `mergeAST` opt-in only #836

Open acao opened 5 years ago

acao commented 5 years ago

As per discussions with @IvanGoncharov, the utility/mergeAst function looks like a good candidate for inclusion in graphql-js itself, however it shouldn't be mutating AST directly. Instead, we will refactor it to use the visit function to accomplish this, and then open a PR to graphql-js. This is just a placeholder for the forthcoming PR

acao commented 5 years ago

The original PR, this is a fairly recent feature:

https://github.com/graphql/graphiql/pull/762

Stumbled across this while trying to convert GraphiQL to typescript!

acao commented 5 years ago

This has turned out to be a much more complicated undertaking than the original scope of this utility, to be feature perfect for all the edge cases of it's original purpose. @IvanGoncharov has provided a lot of insight on this. This current implementation does not take into account aliases, directives, etc. I worry that the "Merge Query" button as it stands, in the current latest release of GraphiQL, could erase part of a user's query.

My PR above is a starting point, but I'll need more time to research things more fully and not waste the graphql-js folks's time until we have a well thought out solution. For the time being, should we consider disabling this "Merge Query" button by default, and making it "opt in" with a warning in the documentation? Or should we wait until it's been added to graphql-js? @benjie , thoughts?

benjie commented 5 years ago

disabling this "Merge Query" button by default, and making it "opt in" with a warning in the documentation

💯 It would be unfair to remove it entirely; let people opt in if they're aware of the issues. In v1.0 this will be a plugin (quite likely a non-default one, I think we should keep our default set of buttons to a minimum).

acao commented 5 years ago

agreed, I think its a powerful feature, but best left disabled by default, given the unintended consequences. that said, I think it's very powerful for projects with complex use of fragments, such as complex gatsby projects, or other large scale graphql schemas

benjie commented 5 years ago

Yeah, it’s great for pinning down issues in large queries as you can do proper bisection which is hard with fragments.