Skip to content

Make mergeAST opt-in only #836

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

Open
acao opened this issue Jun 3, 2019 · 5 comments
Open

Make mergeAST opt-in only #836

acao opened this issue Jun 3, 2019 · 5 comments

Comments

@acao
Copy link
Member

acao commented Jun 3, 2019

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
Copy link
Member Author

acao commented Jun 4, 2019

The original PR, this is a fairly recent feature:

#762

Stumbled across this while trying to convert GraphiQL to typescript!

@acao
Copy link
Member Author

acao commented Jun 4, 2019

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
Copy link
Member

benjie commented Jun 5, 2019

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
Copy link
Member Author

acao commented Jun 5, 2019

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
Copy link
Member

benjie commented Jun 5, 2019

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

@acao acao changed the title Refactor mergeAst to not mutate AST directly Make mergeAST opt-in only Jun 19, 2019
@acao acao added this to the GraphiQL-1.0.0 milestone May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants