Potentially invalid behavior of createTemp

[vladar]

Hey @nodkz 👋

First of all, thank you for this library. It makes our life so much easier in Gatsby. We are in the process of upgrading to the latest v7, so expect some reports 😄

One issue we've hit is that createTemp actually adds the temporary object composer to schema composer. So this test currently passes:

https://github.com/graphql-compose/graphql-compose/blob/08f993f9750d837f00d49efd206177bba02058ae/src/__tests__/ObjectTypeComposer-test.js#L819-L822

But if I add schemaComposer in the second argument - it fails:

it('createTemp() should not store type in schemaComposer', () => {
  const temp = ObjectTypeComposer.createTemp('SomeUser', schemaComposer);
  expect(schemaComposer.has('SomeUser')).toBeFalsy();
  expect(schemaComposer.has(temp.getType())).toBeFalsy();
});

This looks like a bug but maybe I am missing something about createTemp?

[nodkz]

Hi @vladar, great to hear this 🙏

What about the createTemp method - this is expected behaviour. You may see the implementation of createTemp method here https://github.com/graphql-compose/graphql-compose/blob/08f993f9750d837f00d49efd206177bba02058ae/src/ObjectTypeComposer.js#L252-L257

If you do not pass schemaComposer in the second arg, it will create a new one under the hood. If you pass, then it will use the provided schemaComposer instance. For type creation, some type of registry is required; it's used for avoiding stack overflow in circular types that use each other e.g., when you create type User { friends: [User] }.

I don't recommend using the createTemp method. In the past, it was used quite widely in my apps and other packages. But for now how I remember that only graphql-compose-elasticsearch uses it.

For what case do you want to use createTemp? Maybe I can suggest you a better way.

[vladar]

Essentially we use it to merge type definitions. Different plugins may provide different definitions for the same type and we merge those into one type. Intermediate type definitions are created using createTemp and merged into the original definition in schemaComposer. We pass schemaComposer to createTemp because we want to use existing composers for any referenced types (so that we do not need to merge all referenced types as well).

Also, to clarify, we didn't see this behavior in v6 (createTemp didn't seem to add a temporary composer to schema composer)

Take this example (derived from the [actual code in Gatsby](https://github.com/gatsbyjs/gatsby/blob/faf26b2becda8e1a449a0fd7ccf8888377783263/packages/gatsby/src/schema/schema.js#L275-L288 (createTemp is used under the hood in createTypeComposerFromGatsbyType))):

const type = ObjectTypeComposer.createTemp(config, schemaComposer)

const typeName = type.getTypeName()
if (
  // The next line is false in v6, true in v7
  schemaComposer.has(typeName) &&
  // The next line is required for v7 but not v6 to make it work as expected
  type !== schemaComposer.get(typeName)
) {
  const typeComposer = schemaComposer.get(typeName)
  mergeTypes({
    schemaComposer,
    typeComposer,
    type,
    plugin,
    createdFrom,
    parentSpan,
  })
}

[vladar]

Maybe even more clear case: createTemp overwrites the existing type in schemaComposer.

The following test case fails:

      const foo1 = ObjectTypeComposer.create('Foo', schemaComposer);
      const foo2 = ObjectTypeComposer.createTemp('Foo', schemaComposer);

      expect(schemaComposer.get('Foo')).toEqual(foo1)

If I change assertion to expect(schemaComposer.get('Foo')).toEqual(foo2) - it passes.

So it's not clear what is the difference between createTemp and just create in this case.

[nodkz]

According to the provided test case, the problem in these lines https://github.com/graphql-compose/graphql-compose/blob/ffeeab6de8178c7fd34ee96bce308c67caba7ae2/src/ObjectTypeComposer.js#L338-L340

These changes were added in https://github.com/graphql-compose/graphql-compose/commit/ea179e3dfc48e3d4f251e2683a9a88b6d606b0ff according to the problem with merging two schemas with Node interface.

I need to think more about that case and your case. Probably I need to revert that changes and try to figure out the problem with Node interfaces in some different way.

[vladar]

Sounds great!

In the meantime the workaround that kinda works for us:

const type = ObjectTypeComposer.createTemp({
  ...config,
  // setting `fields` as thunk here causes stack overflow for some reason, so no thunk
  fields: schemaComposer.typeMapper.convertOutputFieldConfigMap(config.fields),
  interfaces: () => {
    return (config.interfaces ?? []).map(iface => {
      if (typeof iface === `string`) {
        return schemaComposer.getIFTC(iface).getType()
      } else {
        return iface
      }
    })
  },
})
type.schemaComposer = schemaComposer