Throw better error for missing idType

[brad-decker]

Related to #2281

A better error message for the situation i ran into would have helped me figure out my mistake much faster. This just checks to see if there is no id field defined in the schema (returns undefined) and if so throws a different error message instead of asserting that 'undefined' is a leaf type (Which is most certainly is not).

[brad-decker]

There are some flow errors and what not locally but does not appear to be related to my changes -- also if you'd like a test case written for this message i am all for it and would appreciate a little guidance on where that should be best placed. 👍 Thanks

[alloy]

I’m not sure I understand this. The original code appears to try to get the ID type, shouldn’t that be a builtin type that should always exist?

Regardless, can you use invariant here instead, which appears to be the way assertions are performed throughout the existing code-base?

[alloy]

Ok, I should dive deeper into when the ID type actually exists, as there’s #2285 by @graingert that also mentions this issue.

Interestingly enough, @sibelius mentions that it would be fixed by my #2249 PR, although I didn’t knew of that issue at the time. @sibelius have you verified that? @brad-decker & @graingert would you mind verifying if #2249 fixes your issues and then please leave feedback on the PR? 🙏

[brad-decker]

@alloy -- will do. Thanks for checking this out.

[graingert]

doesn't calling:

context.serverSchema.getType(ID_TYPE);

throw the error?

doing any checks afterwards isn't helpful :(

[brad-decker]

Nope, if the id type isn't there this returns 'undefined' its the assertLeafType below that actually throws an error

[sibelius]

schema without ID that will throw this error https://github.com/freitasgst/weather-express-graphql/commit/c8ee408afc2e5ac4deaeb45d260800040199f57d

[alloy]

@sibelius You indicated here that #2249 fixes it; is that indeed the case or not after all?

[sibelius]

Not after all, this is the right fix for that

[alloy]

Seeing as according to @sibelius this still needs to go in, here’s my review:

• At runtime Relay will generate IDs for records that have none, but I’m not 100% sure off the top of my head if those need the ID type to exist in the schema; so do we even really need to throw an error in the case of no ID type existing?
• If we do indeed need to throw, then please use invariant rather than throw.

[brad-decker]

@alloy in my experience without the ID type the compiler fails, i'm open to other input on whether we need to throw. I'll just go ahead and change to invariant and then we can decide after that whether its required.

[brad-decker]

@alloy @sibelius is there a better way to handle flow typing here. If i don't reassign from the assertLeafType it gives me a flow error because it doesn't know that idType is in fact a LeafType.

[brad-decker]

flow error:

Cannot assign object literal to idField because:
 • in property type:
    • Either GraphQLInputObjectType [1] is incompatible with
      GraphQLScalarType [2].
    • Or GraphQLInputObjectType [1] is incompatible with GraphQLEnumType [3].
    • Or GraphQLInputObjectType [1] is incompatible with GraphQLList [4].
    • Or GraphQLInputObjectType [1] is incompatible with GraphQLNonNull [5].
 • in property type:
    • Either GraphQLInterfaceType [1] is incompatible with
      GraphQLScalarType [2].
    • Or GraphQLInterfaceType [1] is incompatible with GraphQLEnumType [3].
    • Or GraphQLInterfaceType [1] is incompatible with GraphQLList [4].
    • Or GraphQLInterfaceType [1] is incompatible with GraphQLNonNull [5].
 • in property type:
    • Either GraphQLObjectType [1] is incompatible with GraphQLScalarType [2].
    • Or GraphQLObjectType [1] is incompatible with GraphQLEnumType [3].
    • Or GraphQLObjectType [1] is incompatible with GraphQLList [4].
    • Or GraphQLObjectType [1] is incompatible with GraphQLNonNull [5].
 • in property type:
    • Either GraphQLUnionType [1] is incompatible with GraphQLScalarType [2].
    • Or GraphQLUnionType [1] is incompatible with GraphQLEnumType [3].
    • Or GraphQLUnionType [1] is incompatible with GraphQLList [4].
    • Or GraphQLUnionType [1] is incompatible with GraphQLNonNull [5].

        packages/relay-compiler/transforms/RelayGenerateIDFieldTransform.js
         53│   const idType = context.serverSchema.getType(ID_TYPE);
         54│   invariant(idType, 'Schema does not contain an id field on any types');
         55│   assertLeafType(idType);
         56│   const idField: ScalarField = {
         57│     kind: 'ScalarField',
         58│     alias: (null: ?string),
         59│     args: [],
         60│     directives: [],
         61│     handles: null,
         62│     metadata: null,
         63│     name: ID,
         64│     type: idType,
         65│   };
         66│   const state = {
         67│     idField,
         68│   };```

[brad-decker]

the issue was the 'type: idType' assignment

[brad-decker]

@alloy / @sibelius ready for re-review, did something weird to get around a flow issue and would welcome feedback.

[brad-decker]

Checking back in, anything I can do to make this better?

[jstejada]

would splitting this into 2 variables help?

[alloy]

Sorry for missing your question @brad-decker, @jstejada’s suggestion makes sense to me.