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

[Feature] Implment possiblity to hide interface fields on implementations #141

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

matystl
Copy link
Contributor

@matystl matystl commented Dec 4, 2019

Problem this PR tries to solve:
When you have interface with lot of common fields and multiple implementation of that interface currently all fields are shown for every implementation. That unnecessary make graph bigger than it needs to be. See this issue opened for this #41

Solution:
I introduced new boolean in settings panel for hiding fields on objects that are from interface. Default settings is false to be compatible with current behavior. To be still able to tell what fields are available on object i added into graph information about what interfaces are implemented by object(currently it is available in side panel but not it graph itself)

Technical details:
Adding new settings is quite forward.

Then in src/introspection/introspection.ts i added function markInterfaceFields that traverse graph and delete fields from objects that are also present in interfaces.

Additionally in src/graph/dot.ts i added interfacesTypes function that is responsible for rendering (adding graphdot syntax string) list of interfaces that objects implement.

Last important part is in file src/graph/type-graph.ts which is responsible to decide what part of graph should be rendered from root type selected. Currently it search only forward from selected root. That present problem that when objects implement interface that is not reachable from root it is not shown in graph. This is kind of general problem but is really highlighted with this change because when you hide common field you will miss important part of graph(reachable fields are not shown anywhere). There are 2 solution for this problem:

  • allow back propagation from discovered nodes to interfaces and than normal forward propagation from discovered interfaces
  • allow back propagation from discovered nodes to interfaces but not do normal forward propagation from back visited interfaces only forward propagation to fields

Both solution ensure that all important details are shown in graph. I and my 2 coleages are more keen to first one. It is simpler to implement. It shows all information to navigate inside graph and small disadvantage that maybe it will show some nodes that you didn't ask for.

Small remark to authors code base was amazing to work with and i really enjoyed adding this feature.

Picture for TLDR
voyager-show-interface-fields
voyager-hide-iterface-fields

@matystl matystl changed the title Implment possiblity to hide interface fields in object [Feature] Implment possiblity to hide interface fields in object Dec 4, 2019
@matystl matystl changed the title [Feature] Implment possiblity to hide interface fields in object [Feature] Implment possiblity to hide interface fields on implementations Dec 4, 2019
// and also present in object
_.each(type.interfaces, oneInterface => {
_.each(oneInterface.type.fields, field => {
delete type.fields[field.name];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should delete fields only if fully equal to the interface's field.
Counterexample:

interface Foo {
  foo: String
}

type Bar implements Foo {
  foo(arg: Int): String
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also now you can have type without fields please check how they are rendering both in graph UI and left the panel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uff i didn't know that you can override field definition from interface inside concrete implementation.

Type without fields(as they are all same as in interface) is currently ok because i added list of interfaces that concrete type implements
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for left panel it is kind of sad because if you click on Foo type it will show only interface that it implements instead of all fields which should be preferable as there is place for it.

image

Maybe better than deleting fields might be more sensible to add some flag to them and then when they are rendered into graph view they will be hidden

Copy link
Contributor Author

@matystl matystl Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok i have prototype where instead of deleting fields on type (because than we can't show them in docs on left side) i added flag isSameAsOnInterface on field definition and then when it is drawing type box is it reuse canDisplayRow which hides enums and scalars with option showLeafFields with similar logic for fields from interface.

image

Copy link
Contributor Author

@matystl matystl Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And with example

interface Foo {
  foo: String
}

type Bar implements Foo {
  foo(arg: Int): String
}

@IvanGoncharov Do you have idea how condition for checking if field on interface and type are same? Name should be same thats for sure.And then Args should be map with same key? Is there anything else? (optionality, default values)

I checked you cant change type of returned thing.

  interface IFoo {
      foo: String
      bar: String
  }
  
  type Foo implements IFoo {
      foo: String
      bar: Int
  }

Nor you can change type inside arguments like this

  interface IFoo {
      foo: String
      bar(arg:String): String
  }
  
  type Foo implements IFoo {
      foo: String
      bar(arg: Int): String
  }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matystl Here are the rules for implementing interfaces: https://graphql.github.io/graphql-spec/draft/#sel-HAHZhCFHABABiCp7I

@IvanGoncharov
Copy link
Member

@matystl Thanks for PR and detailed explanation 👍

I need to think more about it since it introduces bidirectional relationship between interfaces and types that implement them but we can't render it in the dot.

Another problem is that it provides a very good UX if you came from the interface to type but what if you travel to type directly:

type Query {
  getFoo: Foo
  getBar: Bar
}

interface Foo {
  foo: String
}

type Bar implements Foo {
  foo: String
}

So if you tracing getFoo UX is excellent but I'm not sure it's the same if you tracing getBar.

In general, I need to think about it.
I would travel a lot the next week but will try to give some feedback on this one.
Feel free to ping me if I don't reply for too long.

@matystl
Copy link
Contributor Author

matystl commented Dec 4, 2019

I can't comment to previous comment but here is my brain dump for selection algoritm and pros and cons which different solutions might bring. It is however much bigger topic that probably need to be tacked by this PR ;-)

And to comment about traversal yes current solution is not best. When you navigate directly to type you will not see interfaces that type have or you as we had in our schema interface that is not returned from query you can't find out about it nor from docs in left nor in graph itself (unless you navigate directly to that interface type). So maybe when traversing it might be good idea to show interfaces of types (even when we will not expand that interfaces further).

Similar point can be risen for unions currently there if you navigate to type you have no chance to find out what unions is this type part of.(which is in my opinion smaller problem than interfaces).

If not shown in graph itself it would be cool if left side of docs would be fully navigable as it is currently in graphiql. (current behavior shows only interfaces that are show in graph).

Another long term solution would be to be able dynamicaly add items to graph. by that i mean on left side with docs there is everything. When we render type, parrent interfaces are only shown as list in type but when you click on it it will add it and redraw schema. From there we can show interface with all implementations either fully traversed to leafs or same logic with only list of them in graph and possibility to dynamicaly expanding it.

Dynamic/different selection part of graph might help with showing only part of graph instead of only one current root (something like give me set of types you want to plot in graph instead only one root).

Also it can help with huge schemas. I played with onegraph schema which is like 1.4MB in size and normaly voyager just freeze. When i limit number of shown nodes it was working with no problems. Therefore it might help for large schemas to limit number of defaultly shown nodes and being able to add(navigate) them dynamicaly.

Another cool example why it might be nice to have different selection algorithm is showing schema for particular query. Given query extract types and plot them in voyager with some aditional informations.

Another uscase in my mind is types that have like 50 fields which imediatly break nice flow of graph so instead of hide leaf fields there could be something like defaulty we show only 20 fields and link with ... that would exand that to whole view.

@@ -202,6 +202,46 @@ function markDeprecated(schema: SimplifiedIntrospectionWithIds): void {
// which are deprecated.
}

function markInterfaceFields(schema: SimplifiedIntrospectionWithIds): void {
function isFieldOnInterfaceSameAsOnType(objectField, interfaceField) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IvanGoncharov added check that fields are same. I tested it manually with this schema

    interface IFoo {
      name: String
    }
    type Foo implements IFoo {
      name: String
    }
    type Bar {
      name: String
    }
    union FooAndBar = Foo | Bar

    # same types
    interface I0 {
      name: Foo
    }
    type Example0_hide implements I0 {
      name: Foo
    }

    # possible type of interface
    interface I1 {
      name: IFoo
    }
    type Example11_hide implements I1 {
      name: IFoo
    }
    type Example12 implements I1 {
      name: Foo
    }

    # possible type of union
    interface I2 {
      name: FooAndBar
    }
    type Example21_hide implements I2 {
      name: FooAndBar
    }
    type Example22 implements I2 {
      name: Foo
    }
    type Example23 implements I2 {
      name: Bar
    }

    # list subtyping
    interface I3 {
      name: [IFoo]
    }
    type Example31_hide implements I3 {
      name: [IFoo]
    }
    type Example32 implements I3 {
      name: [Foo]
    }

    # !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
    # stricker nullability
    # !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

    # same types
    interface I4 {
      name: Foo
    }
    type Example4 implements I4 {
      name: Foo!
    }

    # possible type of interface
    interface I5 {
      name: IFoo
    }
    type Example51 implements I5 {
      name: IFoo!
    }
    type Example52 implements I5 {
      name: Foo!
    }

    # same union
    interface I6 {
      name: FooAndBar
    }
    type Example6 implements I6 {
      name: FooAndBar!
    }

    # possible type of union
    interface I7 {
      name: FooAndBar
    }
    type Example71 implements I7 {
      name: Foo!
    }
    type Example72 implements I7 {
      name: Bar!
    }

    # list subtyping
    interface I8 {
      name: [IFoo]
    }
    type Example81 implements I8 {
      name: [IFoo!]
    }
    type Example82 implements I8 {
      name: [Foo!]
    }
    type Example83 implements I8 {
      name: [IFoo]!
    }
    type Example84 implements I8 {
      name: [Foo]!
    }
    type Example85 implements I8 {
      name: [IFoo!]!
    }
    type Example86 implements I8 {
      name: [Foo!]!
    }

    # list subtyping 2
    interface I9 {
      name: [IFoo!]
    }
    type Example91_hide implements I9 {
      name: [IFoo!]
    }
    type Example92 implements I9 {
      name: [Foo!]
    }
    type Example93 implements I9 {
      name: [IFoo!]!
    }
    type Example94 implements I9 {
      name: [Foo!]!
    }

    # list subtyping 3
    interface I10 {
      name: [IFoo]!
    }
    type Example101_hide implements I10 {
      name: [IFoo]!
    }
    type Example102 implements I10 {
      name: [Foo]!
    }
    type Example103 implements I10 {
      name: [IFoo!]!
    }
    type Example104 implements I10 {
      name: [Foo!]!
    }

    # argument list
    interface I11 {
      name(some: String): Foo
    }
    type Example111_hide implements I11 {
      name(some: String): Foo
    }
    type Example112 implements I11 {
      name(some: String, aditionalArg:String): Foo
    }
    

    

    union Result =
        Example0_hide
      | Example11_hide
      | Example12
      | Example21_hide
      | Example22
      | Example23
      | Example31_hide
      | Example32
      | Example4
      | Example51
      | Example52
      | Example6
      | Example71
      | Example72
      | Example81
      | Example82
      | Example83
      | Example84
      | Example85
      | Example86
      | Example91_hide
      | Example92
      | Example93
      | Example94
      | Example101_hide
      | Example102
      | Example103
      | Example104
      | Example111_hide
      | Example112
      
    type Query {
      getFoo: Result
      getFooBar: FooAndBar
      # Get one todo item
    }

    type Mutation {
      addTodo(name: String!): String!
    }

    schema {
      query: Query
      mutation: Mutation
    }

@matystl
Copy link
Contributor Author

matystl commented Jan 6, 2020

@IvanGoncharov Did you have any time to think about what to do with this? :-)

@IvanGoncharov
Copy link
Member

Did you have any time to think about what to do with this? :-)

@matystl Sorry, no. I'm working on long overdue graphql-js v15 release but I will try to find some time afterward.

@dobrynin
Copy link

+1 for wanting a feature like this, although in my case I'd like to hide the fields on the interface rather than the implementing types.

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

Successfully merging this pull request may close these issues.

None yet

3 participants