Check modelDef.getType() is defined before use

[blakeembrey]

It was crashing otherwise. The type format used was directly an alias:

import * as Models from 'some-external-module'

type Foo = Models.Foo

(node:20280) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'kind' of undefined
    at fieldsFromModelDefinition (/Users/blakeembrey/Projects/GitHub/blakeembrey/authkit-service/functions/admin-graphql/node_modules/graphqlgen/dist/generators/common.js:26:27)
    at Object.renderDefaultResolvers (/Users/blakeembrey/Projects/GitHub/blakeembrey/authkit-service/functions/admin-graphql/node_modules/graphqlgen/dist/generators/common.js:40:60)
    at renderNamespace (/Users/blakeembrey/Projects/GitHub/blakeembrey/authkit-service/functions/admin-graphql/node_modules/graphqlgen/dist/generators/ts-generator.js:85:96)
    at /Users/blakeembrey/Projects/GitHub/blakeembrey/authkit-service/functions/admin-graphql/node_modules/graphqlgen/dist/generators/ts-generator.js:80:16
    at Array.map (<anonymous>)
    at renderNamespaces (/Users/blakeembrey/Projects/GitHub/blakeembrey/authkit-service/functions/admin-graphql/node_modules/graphqlgen/dist/generators/ts-generator.js:79:10)
    at Object.generate (/Users/blakeembrey/Projects/GitHub/blakeembrey/authkit-service/functions/admin-graphql/node_modules/graphqlgen/dist/generators/ts-generator.js:51:93)
    at generateTypes (/Users/blakeembrey/Projects/GitHub/blakeembrey/authkit-service/functions/admin-graphql/node_modules/graphqlgen/dist/index.js:84:38)
    at generateCode (/Users/blakeembrey/Projects/GitHub/blakeembrey/authkit-service/functions/admin-graphql/node_modules/graphqlgen/dist/index.js:105:26)
    at /Users/blakeembrey/Projects/GitHub/blakeembrey/authkit-service/functions/admin-graphql/node_modules/graphqlgen/dist/index.js:195:26

[jasonkuhrt]

Thanks @blakeembrey!

1. Could you give me a bit more hint on how to repro
2. Could you add a test case for this in common.spec.ts
3. Also would like the static types improved so the evident nullability is modelled

I'll try to contribute commits helping each point when I find time. Feel free to do it sooner if you wish
: )

[blakeembrey]

@jasonkuhrt It's replicated like in the PR description. I narrowed it down to buildTypeGetter and it should be really simple to replicate. It's because the InternalInnerType === { kind: 'TypeReferenceAnnotation', referenceType: undefined }. The referenceType === undefined does not exist in the file so it fails.

[ksaldana1]

This will just fix the crashing behavior, not actually add support for type alias syntax for models, right? I had to add this fix inline when first playing with the tool, but my aliased model (in OP example Foo) was not imported in my generated files. The resolver types referred to the correct name, but the import was left off so the typings were invalid.

Relevant #282

[blakeembrey]

@ksaldana1 Yes, exactly. I think solving that might be a much more complex problem. I believe the current TS implementation is a parser and not a full language services which means it doesn't support following and resolving imports (correct me if I'm wrong).

[ksaldana1]

Yes that seems to be correct @blakeembrey . It is using @babel/parser and the AST utils around that. I'm not as familiar with it as I am with the TS compiler, so I'll have to look into it a bit.

[ksaldana1]

Thanks for the cleanup! I probably should have looked a little higher level than my as bike-sheds. I think this code should probably look something more like:

// we already know that modelDef.kind === 'TypeAliasDefinition' here
const interfaceDef = modelDef.getType()
if (interfaceDef && interfaceDef.kind === 'AnonymousInterfaceAnnotation') {
    return interfaceDef.fields
}
return []

Kinda irrelevant though because it seems these types aren't fully capturing everything being passed in at runtime. Don't think it's worth hanging up on code hygiene until that's sorted out.

[jasonkuhrt]

Thanks @blakeembrey, @ksaldana1 I better understand what's going on now. This PR achieves its direct goal as well as removing some useless/unsafe type casting. Let's ship this.

I am keen to dig into the underlying issues in further PRs, namely:

1. type aliases not fully supported
2. we have a runtime optional type not captured in the static type

[blakeembrey]

@jasonkuhrt The runtime issue comes from:

  if (type.kind === 'TypeReferenceAnnotation') {
    return () => filesToTypesMap[filePath][type.referenceType]
  } else {
    return () => type
  }

Specifically, type.referenceType === undefined so it's not in that map. This is because of the index signature for TypesMap, a quick and easy workaround may be to add | undefined to that signature. I started to this sort of tidy up throughout the codebase but it took a bit more work than I had time.