Mongoose discriminantors, composeWithMongooseDiscriminators

[mernxl]

Was able to implement the interface concept minimally, check this issue out #78
Guess we can start the polishing from here.

[nodkz]

Wow, thanks! 👍
Try to review closely it tomorrow. Sorry for waiting, too busy days.

[mernxl]

Well I am using the hack already, found many issues. Well few explanations too. I would be happy if we could make the hack better, the minor implementation is making my work really easy. 🤓🤓

[nodkz]

Made some eslint and flowtype fixes to your branch.
On the next week starts to simplify your code.

[nodkz]

We should avoid using export const EMCResolvers = { here.
If you want to create such mapping, you need to do it in https://github.com/graphql-compose/graphql-compose-mongoose/blob/master/src/resolvers/index.js

I'll refactor connection and pagination resolvers in upcoming days, so all resolvers will be under /src/resolvers folder.

[nodkz]

@mernxl Moved connection and pagination resolvers to the /src/resolvers folder.
yarn install is required! Was updated graphql-compose-pagination and graphql-compose-connection packages.

[nodkz]

It's very bad to automatically add fields to Query type. Developers should to it manually in their code. In my schema I'm using resolvers under my, admin, public fields in the Query (they have different permissions and a different set of resolvers (eg. I do not use pagination, remove* resolvers under Query.public)

[mernxl]

Will work on removing.

[mernxl]

@nodkz, i have this issue with the discriminators. When you don't add a particular discriminator type to query, mutation or subscription, when resolving the Interface, Graphql throws an Error if it references that type which is not in schema.

I believe this explains it more...

In SchemaComposer.js

buildSchema(): GraphQLSchema {
    const roots = {};

    if (this.has('Query')) {
      const tc = this.getTC('Query');
      this.removeEmptyTypes(tc, new Set());
      roots.query = tc.getType();
    }

    if (this.has('Mutation')) {
      const tc = this.getTC('Mutation');
      this.removeEmptyTypes(tc, new Set());
      roots.mutation = tc.getType();
    }

    if (this.has('Subscription')) {
      const tc = this.getTC('Subscription');
      this.removeEmptyTypes(tc, new Set());
      roots.subscription = tc.getType();
    }

    if (Object.keys(roots).length === 0) {
      throw new Error(
        'Can not build schema. Must be initialized at least one ' +
          'of the following types: Query, Mutation, Subscription.'
      );
    }

    return new GraphQLSchema(roots);
  }

While seeking you opinion on how to solve the issue, i am proposing to add a field called _disTypes that contains all types that are either Discrimator or Child of Discriminator to rootQuery. It only host the Types, cannot be Queried, nor Mutated.

In composeWithMongooseDiscriminators.js

if (!this.GQC.rootQuery().hasField('_disTypes')) {
      this.GQC.rootQuery().addFields({
        _disTypes: TypeComposer.create({
          name: '_disTypes',
          description: 'Hoisting for Discriminator Types',
        }),
    });
}

this.GQC.rootQuery()
    .getFieldTC('_disTypes')
    .addFields({
      [this.getTypeName()]: this,
    });

[nodkz]

Can you add a broken test case without _distTypes logic? I can solve this problem today after several hours. I'm very dislike any pollution to the users' schemas.

[mernxl]

Added the test case, there are three test cases,

in src/tests/_disTypes-test.js

1. Test that doesn't include _distTypes but manually i manually added resolvers that return all the Types that implement DInterface
2. Test that includes _disTypes by setting discriminator test_disTypes option to true. This case executes without errors
3. Test that turns test_disTypes option to false, the case returns GraphqlError that i asserted.

[nodkz]

Your tests help me very well to figure out the problem.

I found a solution. But it required to wait for some time while I make proper changes in graphql-compose package.

Just added InterfaceTypeComposer. It still in progress in a separate branch.

After that, it is necessary to teach TypeComposer to work with InterfaceTypeComposer.
And then make small changes in SchemaComposer. Midnight is already, try to do it tomorrow.

---

@mernxl while you wait for me, you may suggest some new methods to graphql-compose which it would be good to have, based on the experience of writing discriminators. If there are any ;)

Also, you may add some info about how discriminators work to README.md.

[mernxl]

@nodkz, great work on InterfaceTypeComposer. Don't know of those new methods as of now, but if they do pop up i will do a PR.

• Will be adding the info on how discriminators work to README.md.

• While working on discriminators, i wrote a small function that merges two TypeConverterOptions, that is takes the first option and merges it with the second. The first opts only replaces the undefined opts in the second arg. I think it will be nice to expose this function so someone could create base type converter options and only merge them with extras when he needs them.

const baseCustomOptions: TypeConverterOpts = {
    fields: baseFields,
    inputType: {
      name: 'BaseInput',
      description: 'Hello Base',
      fields: baseInputTypeFields,
    },
    resolvers: {
      findMany: {
        limit: { defaultValue: 20 },
        // sort: false,
        skip: false,
        filter: {
          isRequired: true,
          removeFields: ['id', 'dob'],
          operators: {
            one: ['gt', 'gte', 'lt'],
            two: ['gt', 'gte', 'lt', 'in[]', 'nin[]'],
          },
        },
      },
      findById: false,
    },
  };

  const childCustomOptions: TypeConverterOpts = {
    fields: childFields,
    inputType: {
      name: 'ChildInputs',
      description: 'Hello Child',
      fields: childInputTypeFields,
    },
    resolvers: {
      findMany: {
        limit: { defaultValue: 50 },
        sort: false,
        // skip: false,
        filter: {
          removeFields: ['gender', 'dob', 'age'],
          operators: {
            one: ['gt', 'lte', 'ne', 'in[]', 'nin[]'],
            two: ['gt', 'gte', 'lt', 'lte', 'ne'],
            three: ['gte', 'lt'],
          },
        },
      },
      updateById: {
        input: {
          removeFields: ['one', 'two', 'five'],
          requiredFields: ['eight', 'two', 'five'],
        },
      },
    },
  };

  const expected: TypeConverterOpts = {
    fields: expectedFields,
    inputType: {
      name: 'ChildInputs',
      description: 'Hello Child',
      fields: expectedInputTypes,
    },
    resolvers: {
      findMany: {
        limit: { defaultValue: 50 },
        sort: false,
        skip: false,
        filter: {
          isRequired: true,
          removeFields: ['id', 'dob', 'gender', 'age'],
          operators: {
            one: ['gt', 'gte', 'lt', 'lte', 'ne', 'in[]', 'nin[]'],
            two: ['gt', 'gte', 'lt', 'in[]', 'nin[]', 'lte', 'ne'],
            three: ['gte', 'lt'],
          },
        },
      },
      findById: false,
      updateById: {
        input: {
          removeFields: ['one', 'two', 'five'],
          requiredFields: ['eight', 'two', 'five'],
        },
      },
    },
  };

  it('should merge customisation Options', () => {
    expect(mergeCustomizationOptions(baseCustomOptions, childCustomOptions)).toEqual(expected);
  });

[nodkz]

Sure, lets expose mergeCustomizationOptions method in this package.

[nodkz]

Why order matters?

[mernxl]

Actually, the recomposeChildResolvers references the DiscriminatorTC, so I felt putting recomposeChildResolvers before setting the child DiscriminatorTC reference might yield errors.
But I have found a way to avoid all those issues.

[nodkz]

graphql-compose-mongoose generates tons of types and resolvers. Developers pick only what they need in their schemas and manually add to Query type in their code or relate with other types. We should not pollute their Query, Mutation types.

[mernxl]

Ok, will look for a means of hoisting those types. I noticed something, when the developer doesn't add those types, they tend to error out when referenced by the DInterface.

[nodkz]

Do we really need this ChildDiscriminatorTypeComposer class? I think that standard TypeComposer will be enough for child models.

[mernxl]

Will refactor the childDTC, to TypeCompose.

[nodkz]

recomposeBaseResolvers.js is a not a good name.
Maybe prepareResolversForDiscriminator.js or something shorter. But from files tree, it should be clear what logic put inside.

[nodkz]

Or maybe better to create a /discriminators folder and put all files into it. So all logic and tests will be in one place.

[nodkz]

Please move DiscriminatorTypeComposer class to its own file src/discriminators/DiscriminatorTypeComposer.js

[nodkz]

@mernxl I think now we have a working solution. 🎉

Ok, it's time to clean up and polish the code/tests. Remove old unhelpful comments. Add an example to README.md Also try to remove one-time used functions (like I did with replacing call of .getGQC() on property reading .schemaComposer).

When you do it, please ping me. After that, I'll review code again, make last corrections and merge PR.

[nodkz]

Here is an error.
You completely replace all fields in childTC, but it may have it own additional types. Code should be fixed in such way:

-      childTC.setField(fieldName, fieldConfig);
+      // childTC may have additional fields, so replace only Base fields
+      childTC.removeField(super.getFieldNames());
+      childTC.addFields(fields);

Also need tests for your methods.

[nodkz]

@mernxl I refactor some things. For now, need to go. Tomorrow will continue.

Current problems:

• missing src/discriminator/__tests__/DiscriminatorTypeComposer-test.js file. You need to move some test cases from src/discriminators/DiscriminatorTypeComposer.js
• src/__tests__/_disTypes-test.js should be renamed or placed to src/discriminators folder. Or it's test cases moved in other files.
• src/index.js does not expose composeWithMongooseDiscriminators function
• maybe you find some old or unworked test cases or comments. Please take a look again on your test cases and think about proper placing at correct files (eg how it happens with DiscriminatorTypeComposer class test cases).

Please fix them. Thanks!
We almost ready for PR, the code became much much more production ready. 💪

[mernxl]

Hey @nodkz, will work on those points.

What about generating Typescript types? I mostly use typescript.
I saw graphql-compose have types, were they generated manually or auto? What ever method, how can we have them in this package please.

[nodkz]

We add typescript defs manually. And then keep them in sync, when introduce new features or make changes for existed methods.

If will be automated method will be good.

In graphql-compose type defs were added in this PR graphql-compose/graphql-compose#83

Feel free to add .d.ts files to this package.

[mernxl]

ok, will work on that on a new PR when done here.

[mernxl]

Getting some flow errors, you might want to check please.
From lines 22, 23 ...
Error:(22, 68) Cannot call expect.arrayContaining with CharacterDTC.getFieldNames() bound to value because string [1] is incompatible with mixed [2] in array element.

[nodkz]

🎉 This PR is included in version 4.4.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[nodkz]

@mernxl thanks for amazing work!

Just add a Live Demo

For any additional changes feel free to open a new branch and PR.

[mernxl]

:) thank you too @nodkz for being a great leader, and for creating this framework. I am happy i could contribute.