Skip to content

Refactor resolver creation #263

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

Closed
nodkz opened this issue Sep 8, 2020 · 5 comments
Closed

Refactor resolver creation #263

nodkz opened this issue Sep 8, 2020 · 5 comments
Milestone

Comments

@nodkz
Copy link
Member

nodkz commented Sep 8, 2020

This discussion was started here: graphql-compose/graphql-compose#281 (comment)

I was working under a new major version of graphql-compose-mongoose in my free time. And there was added 8 new resolvers and two old resolvers were changed with breaking changes. So according to these changes, I feel that storing resolvers in ObjectTypeComposer has the following disadvantages:

  • no static analysis for resolver names
  • no static analysis for available args when you are crating relations
  • quite awkward mechanics of creating new resolvers and wrapping them
  • no go-to support in IDE via ctrl+click for opening resolver definition

There is a long way but something already has done:

  • There were improved typescript definitions. Now generated Resolvers know source: Model<IDoc>
  • Created a new method composeMongoose which does not generate resolvers.
    • It adds mongooseResolvers property to TypeComposer with resolver generators. And when you need a resolver you may generate it in such way PostTC. mongooseResolvers.findMany() (before was PostTC.getResolver('findMany')).
    • Generating resolvers on demand allows you:
      • Modify TypeComposer in a programmatic way and only after that generate resolvers with customized fields
      • Safe memory and time on bootstrap server phase, because won't b generated unused resolvers
      • You may call resolver generators several times with different configurations.

And for now looks so:

import { schemaComposer, graphql } from 'graphql-compose';
import { composeMongoose } from 'graphql-compose-mongoose';
import mongoose, { Document } from 'mongoose';

const UserSchema = new mongoose.Schema({
  name: { type: String, required: true },
});

interface IUser extends Document {
  name: string;
}

const PostSchema = new mongoose.Schema({
  title: { type: String, required: true },
  authorId: { type: mongoose.Types.ObjectId },
  reviewerIds: { type: [mongoose.Types.ObjectId] },
});

interface IPost extends Document {
  title: string;
  authorId?: mongoose.Types.ObjectId;
  reviewerIds?: [mongoose.Types.ObjectId];
}

const UserModel = mongoose.model<IUser>('User', UserSchema);
const PostModel = mongoose.model<IPost>('Post', PostSchema);

const UserTC = composeMongoose(UserModel);
const PostTC = composeMongoose(PostModel);

PostTC.addRelation('author', {
  resolver: UserTC. mongooseResolvers.dataLoaderLean(), // <------ added `mongooseResolvers` with all resolvers
  prepareArgs: {
    _id: (s) => s.authorId,
  },
  projection: { authorId: true },
});

PostTC.addRelation('reviewers', {
  resolver: UserTC. mongooseResolvers.dataLoaderManyLean(), // <------ added `mongooseResolvers` with all resolvers
  prepareArgs: {
    _ids: (s) => s.reviewerIds,
  },
  projection: { reviewerIds: true },
});

schemaComposer.Query.addFields({
  posts: PostTC. mongooseResolvers.findMany(), // <------ added `mongooseResolvers` with all resolvers
});

const schema = schemaComposer.buildSchema();

UPD: in the code above generateResolver was renamed to mongooseResolvers.

@nodkz nodkz added this to the 9.0.0 milestone Sep 8, 2020
nodkz added a commit that referenced this issue Sep 10, 2020
…er without resolvers. Now Resolvers can be generated on-demand in such way `PostTC.generateResolver.findMany()` (before was `PostTC.getResolver('findMany')`).

related #263
@nodkz
Copy link
Member Author

nodkz commented Sep 10, 2020

@toverux how do you think - generateResolver is a good name or not? Maybe you can suggest a better name...

PostTC.addRelation('reviewers', {
  resolver: UserTC.generateResolver.dataLoaderManyLean(), // <------ added `generateResolver` with all resolvers
  prepareArgs: {
    _ids: (s) => s.reviewerIds, // <-------  `s` infers its type `IPost` automatically
  },
  projection: { reviewerIds: true },
});

The whole example you may see above.

@toverux
Copy link
Contributor

toverux commented Sep 11, 2020

Awesome work! Increased type safety and performance will be much welcomed.

generateResolver as a property is strange because it looks like a method name/something that performs an action.

I'd rather call it something like resolversFactory, resolversGenerator or just resolvers.

But you may also want to give it a name that is specific to graphql-compose-mongoose, right? Like mongooseResolvers or something, unless other plugins could extend this object again in turn, but there's a risk of collision.

And there was added 8 new resolvers

That looks exciting! I see a new dataLoaderManyLean ; I'm curious, what are the other ones and will there be non-lean variants (if that's what I think it is, ie. mongoose lean documents, maybe we could configure that with a { lean: boolean } option)?

@nodkz
Copy link
Member Author

nodkz commented Sep 12, 2020

@toverux { lean: boolean } option very good suggestion – opened issue for this feature #266

Yep, I also dislike generateResolver name. And I was right for asking your advise – mongooseResolvers is the best name! In our apps, we are using different data sources so visibility will be greatly improved for developers with UserTC.mongooseResolvers.find(), UserTC.elasticResovers.find().

nodkz added a commit that referenced this issue Sep 12, 2020
If our app is using different data sources so visibility will be greatly improved for developers with `UserTC.mongooseResolvers.find()`, `UserTC.elasticResovers.find()`.

see #263 (comment)
nodkz added a commit that referenced this issue Sep 12, 2020
…er without resolvers. Now Resolvers can be generated on-demand in such way `PostTC.generateResolver.findMany()` (before was `PostTC.getResolver('findMany')`).

related #263
nodkz added a commit that referenced this issue Sep 12, 2020
If our app is using different data sources so visibility will be greatly improved for developers with `UserTC.mongooseResolvers.find()`, `UserTC.elasticResovers.find()`.

see #263 (comment)
@nodkz
Copy link
Member Author

nodkz commented Sep 12, 2020

You may try a new version v9.0.0-alpha.6.
As a playground, you may use this test suite https://github.com/graphql-compose/graphql-compose-mongoose/blob/alpha/src/__tests__/github_issues/141-test.ts

In v9 will be kept to ways of generation TypeComposer via

I suppose that you'll very like testFieldConfig helper (execute integration graphql query without blowing the code). For field property you may pass any graphql field config object. With Resolvers it works a little better - it has simple auto-complete of args names and checks for required args:

await testFieldConfig({
field: ComplexTC.mongooseResolvers.findById(),
args: { _id: { region: 'us-west', zone: 'a' } },
selection: `{
_id {
region
zone
}
name
}`,
})
).toEqual({ _id: { region: 'us-west', zone: 'a' }, name: 'Compute' });

@toverux Please take a look. If you will have any ideas for issues - please let me know.

@nodkz
Copy link
Member Author

nodkz commented Sep 26, 2020

@toverux can I ask you to review release notes for v9.0.0?

If you will have any questions, additions, or fixes – please write them here (or you may make fixes via PR for the alpha branch).
Thanks.

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