OneGraph / graphiql-explorer

Explorer plugin for GraphiQL
MIT License
904 stars 97 forks source link

Operation transforms #44

Closed sgrove closed 4 years ago

sgrove commented 4 years ago

YouTube Preview Pretty big feature additions:

  1. Clone/destroy operations
  2. Extract fragments
  3. Inline/extract variables

2 has to do non-local changes to the full GraphQL doc (inserting new operations, reading other operations to find conflicting names), and both #2 and #3 have to do two steps (replace the immediate selection value with something, and either remove/add another e.g. variable/fragment).

I changed the signature of most modifySelection-like functions to take a second argument, commit : boolean. If this is false, it will run the transformation and - if successful - return the full, newly-created graphql doc.

I also threaded an additional prop, onCommit through the components, which will take any full GraphQL doc and notify the outside world of the change. This way both #2 and #3 can do their non-local operations, and they also get transactionality.

There's quite a bit of nuance to get this to feel right, e.g. when extracting a variable that already has a variable somewhere underneath, we need to remove that variable, and in the process may make it unused, so we also need to remove it if it's no longer referenced in the query. Not everything is perfect, but it functionally it feels pretty good right now.

FluorescentHallucinogen commented 4 years ago

@sgrove It's not clear from the video:

  1. Is it possible to inline fragments or only extract to fragments?

  2. Is it possible to extract objects (non-leaf fields that has child/nested fields) to variables or only leaf fileds?

I'm also concerned that Explorer extracts input fields into default variable values, not into standalone reusable variables, but extract output fields into standalone reusable named fragments, not into inline fragments.

What if the user created an operation and extracted some fields into fragment? After that he cloned this operation. As a result, both operations use the same fragment. If the user modifies the fragment, this affects both operations. This may be not at all what the user would like.

But on the other hand in some use cases this is exactly what is needed.

It's the same for variables.

Ideally, there should be 4 different actions: "Extract into named fragment", "Extract into inline fragment", "Extract into standalone variable", "Extract into variable default value" to cover all use cases.

BTW,

  1. Is it possible to create nested fragments?

  2. Is it possible to extract variables inside fragments?

FluorescentHallucinogen commented 4 years ago

As an idea, other option is to give unique names to extracted variables and fragments (e.g. prefix variable and fragment names by operation name) to avoid the problem of editing shared variables and fragments for different operations.

sgrove commented 4 years ago
  1. It allows only for extracting fragments, not inlining. A feature to be added later, probably.
  2. Variablizing a non-leaf node works, with some nuance (any variables that were in sub-selections will be inlined, and if they end up being unused in the query afterwards, they'll be removed from the definition.

Nested fragments work. Extracting variables inside fragments doesn't.

The operation-name-prefix for newly extracted fragments might work, I'll think it over.

Mainly the goal of this PR is to make powerful operations available to those who know how to use them, without causing new users to fall into any invalid/hard-to-recover-from situations. So in your "shared-fragment" example, it's likely what a new user would want, and a power-user knows how to edit the query by hand.

And the explorer isn't meant to replace the text editor entirely (at least, not yet!), so it's ok to punt for more specific kinds of transformations right now.

We'll keep adding on/polishing as our time allows.

FluorescentHallucinogen commented 4 years ago

@sgrove Thanks for your answers!

Mainly the goal of this PR is to make powerful operations available to those who know how to use them, without causing new users to fall into any invalid/hard-to-recover-from situations.

And the explorer isn't meant to replace the text editor entirely (at least, not yet!)

My vision is Explorer is just another (visual) representation of code. And everything that could be possible via code should be possible via Explorer, and vice versa. I think the main goal of Explorer is that it allows users to build full GraphQL queries without typing a single line of code.

There are some pain points for new GraphQL users. Specifically union types and inline fragments. If the user is not aware of this syntax to query this type of fields it can be a fairly frustrating experience! Explorer helps solve this problems. This enables users that don’t yet fully understand the GraphQL query syntax to learn GraphQL much more easily.

So ideally Explorer should make powerful operations available to those who don't know how to use them, not to only those who know. ;)

@sgrove BTW, have you ever considered that Explorer could replace the "Docs" panel in the upper right-hand corner of GraphiQL in the long term? Both allow overview (explore) the GraphQL API.

FluorescentHallucinogen commented 4 years ago

@sgrove I've tried to build the code from this PR, but get the following error:

SyntaxError: src/Explorer.js: Unexpected token (2202:13)
  2200 |           )}
  2201 |           {!!this.state.displayTitleActions ? (
> 2202 |             <>
       |              ^
  2203 |               <button
  2204 |                 type="submit"
  2205 |                 className="toolbar-button"
FluorescentHallucinogen commented 4 years ago

@sgrove Any chance you'll finish this in the foreseeable future?

dwwoelfel commented 4 years ago

Not a fan of this onCommit thing. I think we're going to regret trying to shoehorn a mechanism for global changes into something designed to do local changes. Would much rather pass an editDocument function down to the components so that they can do global changes. I won't block on it, though, in the interest of moving quickly.

sgrove commented 4 years ago

I agree re: onCommit- the plan is to rewrite the majority of this using visitors in the future.