Skip to content

Improve Resolvers plugins #1593

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

Merged
merged 7 commits into from
Apr 1, 2019
Merged

Improve Resolvers plugins #1593

merged 7 commits into from
Apr 1, 2019

Conversation

dotansimha
Copy link
Owner

@dotansimha dotansimha commented Mar 28, 2019

This PR will resolve the following issues related to resolvers packages:

@jonaskello
Copy link
Contributor

Nice work!!
I have a question about how the replace using Omit works.
I think the idea is to make the types recursivly defined by ResolversTypes?

So with this schema:

type MyType {
    foo: String!
    otherType: MyOtherType
  }
type MyOtherType {
    bar: String!
}
type Query {
    something: MyType!
}

We would by default get:

type ResolversTypes = {
    Query: Omit<Query, 'something'> & { something: ResolversTypes['MyType'] },
    MyType: Omit<MyType, 'otherType'> & { otherType: Maybe<ResolversTypes['MyOtherType']> },
    String: Scalars['String'],
    MyOtherType: MyOtherType,
}

This looks awesome since the return values of the resolvers now are recursively defined with it's own types instead of the original schema types.

My question is how we would make a Partial of MyType above without loosing the recursive definition?
I could add this mappers config:

MyType: 'Partial<MyType>'

But I guess then I would get:

type ResolversTypes = {
    Query: Omit<Query, 'something'> & { something: ResolversTypes['MyType'] },
    MyType: Partial<MyType>,
    String: Scalars['String'],
    MyOtherType: MyOtherType,
}

So I would get a partial of the orignal MyType schema type where MyType.otherType is of type MyOtherType instead of ResolversTypes['MyOtherType']. What I really wanted was a partial of the recursively defined resolver type. Something like this:

type ResolversTypes = {
    Query: Omit<Query, 'something'> & { something: ResolversTypes['MyType'] },
    MyType: Partial<Omit<MyType, 'otherType'> & { otherType: Maybe<ResolversTypes['MyOtherType']> }>,
    String: Scalars['String'],
    MyOtherType: MyOtherType,
}

I'm not sure how but perhaps it would be possible to make the recursively defined resolver types available for additional mapping? Or maybe I am misunderstanding how it is supposed to work. Anyway, keep up the good work :-).

Copy link
Collaborator

@kamilkisiela kamilkisiela left a comment

Choose a reason for hiding this comment

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

Looks good, except it doesn't set a custom type on a nested usage when defaultMapper is Partial<{T}>. I added a test for that

@jonaskello
Copy link
Contributor

Regarding my previous comment, perhaps something like this would be possible. Types specified in mappers is put in MapperTypes to not overwrite the default/original resolver type which has the Omit stuff. Types in ResolversTypes use MapperTypes if available, otherwise ResolversTypes.

export type MapperTypes =  {
  readonly MyType: Partial<ResolversTypes["MyType"]>,
  readonly MyOtherType: ResolversTypes["MyOtherType"],
}

export type ResolversTypes = {
  readonly Query: Omit<Query, 'something'> & { readonly something: MapperTypes['MyType'] },
  readonly MyType: Omit<MyType, 'otherType'> & { readonly otherType: Maybe<MapperTypes['MyOtherType']> },
  readonly String: string,
  readonly MyOtherType: MyOtherType,
};

I don't really like it though since it becomes more complex and also it is not clear if types from MapperTypes or ResolversTypes should be used as return types for resolvers.

@dotansimha
Copy link
Owner Author

@jonaskello We aim for replacing a type with another type in the level of its declaration, and not in the level of its usage, because we don't want to map fields, but types.
So if you map MyType: 'Partial<MyType>', then MyType will get replaced by Partial<MyType>, but anywhere you use it, and not in a specific usage of it.
Mapping fields inside types might be too complicated.

Regarding MapperTypes - it might get too complicated and hard to understand. Our goal it so simplify the usage and map each GraphQL type to a single type.

The priority ResolversTypes's fields is: look for mappers, then look for default mapper, then look for scalar, otherwise, use the type generated by typescript.

@dotansimha dotansimha requested a review from kamilkisiela March 31, 2019 08:26
@jonaskello
Copy link
Contributor

@dotansimha Yes, I agree on the added complexity. Ideally we would have the possibility to use the Omitted types without adding complexity but I don't see a solution for that right now. Anyway, the changes in this PR is a big step forward for resolver typings without that possibility 👍

@kamilkisiela
Copy link
Collaborator

Looks good 👍

@mshwery
Copy link

mshwery commented Apr 5, 2019

Looks like this hasn't landed yet. Any idea on when the next release will be?

@dotansimha
Copy link
Owner Author

Hi @mshwery
We have auto-release for canary versions on each change, if you wish to try those new features now, you can use: 1.0.7-alpha-e006b14b.26.

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.

4 participants