-
-
Notifications
You must be signed in to change notification settings - Fork 517
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
base: main
Are you sure you want to change the base?
Conversation
src/introspection/introspection.ts
Outdated
// and also present in object | ||
_.each(type.interfaces, oneInterface => { | ||
_.each(oneInterface.type.fields, field => { | ||
delete type.fields[field.name]; |
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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
@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 In general, I need to think about it. |
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 |
@@ -202,6 +202,46 @@ function markDeprecated(schema: SimplifiedIntrospectionWithIds): void { | |||
// which are deprecated. | |||
} | |||
|
|||
function markInterfaceFields(schema: SimplifiedIntrospectionWithIds): void { | |||
function isFieldOnInterfaceSameAsOnType(objectField, interfaceField) { |
There was a problem hiding this comment.
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
}
@IvanGoncharov Did you have any time to think about what to do with this? :-) |
@matystl Sorry, no. I'm working on long overdue |
+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. |
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 functionmarkInterfaceFields
that traverse graph and delete fields from objects that are also present in interfaces.Additionally in
src/graph/dot.ts
i addedinterfacesTypes
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: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