MichalLytek / type-graphql

Create GraphQL schema and resolvers with TypeScript, using classes and decorators!
https://typegraphql.com
MIT License
8.03k stars 676 forks source link

Runtime error when putting type-graphl types into same file in new graphql-code-generator plugin #380

Closed borremosch closed 5 years ago

borremosch commented 5 years ago

I have submitted a pull request in the graphql-code-generator repository that updates it with the ability to generate type-graphql decorated classes. I need this functionality because I am working on a service that consumes data from another GraphQL service, and then relays some of the functionality in its own GraphQL server. The pull request can be found here.

I have got it to a point where it generates all required type-graphql classes, interfaces, enums etc. into compilable code. For example:

import * as TypeGraphQL from 'type-graphql';

[...]

@TypeGraphQL.ObjectType()
export class Config {
  __typename?: 'Config';

  @TypeGraphQL.Field(type => Runtime)
  runtime!: Runtime;
}

@TypeGraphQL.ObjectType()
export class Runtime {
  __typename?: 'Runtime';

  @TypeGraphQL.Field(type => String)
  name!: Scalars['String'];

  @TypeGraphQL.Field(type => String)
  version!: Scalars['String'];
}

The problem with the example above, however, is that there are runtime errors caused by the compiled TypeScript decorators. One of the decorators metadata will try to access a class that is defined later in the same file, and crash because it is undefined, e.g.:

/usr/src/app/build/main.js:2261
    __metadata("design:type", Runtime)
                              ^

ReferenceError: Runtime is not defined

I am hoping that there is a solution in which the classes do not have to split up over multiple files, as this would require a major refactor of graphql-code-generator.

MichalLytek commented 5 years ago

Unfortunatelly, this is how JS works.

const b = {
  a,
  b: "b",
};

const a = {
  a: "a",
  b,
};

console.log(a); // { a: 'a', b: { a: undefined, b: 'b' } } 
console.log(b); // { a: undefined, b: 'b' } 

Mongoose models suffers from the same problem - you have to split the declarations to a separate file to make the Node.js require do the magic with lazy loading circular references.

MichalLytek commented 5 years ago

However, you might try to wrap it into a namespace and then refer the classes by NamespacePrefix.Classname - it will do the same trick like with modules loading mechanism, however I haven't tested that yet.

borremosch commented 5 years ago

Thanks for the namespaces tip, I will check it out.

I might have found another solution that works. By adding a dummy type FixDecorator to the code like so:

import * as TypeGraphQL from 'type-graphql';
type FixDecorator<T> = T;

[...]

@TypeGraphQL.ObjectType()
export class Config {
  __typename?: 'Config';

  @TypeGraphQL.Field(type => Runtime)
  runtime!: **FixDecorator**<Runtime>;
}

@TypeGraphQL.ObjectType()
export class Runtime {
  __typename?: 'Runtime';

  @TypeGraphQL.Field(type => String)
  name!: Scalars['String'];

  @TypeGraphQL.Field(type => String)
  version!: Scalars['String'];
}

It compiles and runs just fine. That is because the compiled code changes from

__decorate([
    TypeGraphQL.Field(type => Runtime),
    __metadata("design:type", Runtime)
], Config.prototype, "runtime", void 0);

to

__decorate([
    TypeGraphQL.Field(type => Runtime),
    __metadata("design:type", Object)
], Config.prototype, "runtime", void 0);

Do you reckon that the generic design:type for this type will have any negative impact on type-graphql? After all, the metadata design:type will be Object in many cases, most notably when a type is nullable.

MichalLytek commented 5 years ago

Do you reckon that the generic design:type for this type will have any negative impact on type-graphql?

In generated classes? No, it will work fine. Reflection is only for better UX so you don't have to annotate each field twice.

borremosch commented 5 years ago

That should be an ok solution for now then. As the types are auto-generated, the duplicate annotation will not be much of a UX issue in this particular case. Thanks for the help, I will close this issue now.