Circular dependencies bug

[davewasmer]

Describe the bug
The current implementation of the Field decorator leads to attempts to synchronously load circular dependencies, resulting in imported modules being undefined.

To Reproduce
https://github.com/davewasmer/typegraphql-circular-deps-test
Clone down, npm/yarn install, then run either:

• yarn ts-node src/foo.ts, or
• yarn ts-node src/bar.ts

The console output will show that the type function passed into @Field is invoked synchronously, resulting in undefined for the imported file.

Expected behavior
The @Field decorator would invoke the type function lazily (i.e. not in the synchronous load path of the module), allowing for properly defined circular dependencies.

Enviornment (please complete the following information):

• OS: Mac Mojave
• Node: 11.1
• Package version: master
• TypeScript version: 3.2.2

Additional context
There's a few relevant pieces of prior art here:

• Support for circular references #130 - Support for circular references
• tests/functional/circular-refs.ts

Both of these are concerned primarily with: "Can Type-GraphQL build an accurate schema with circular deps?" The answer is yes, as the tests demonstrate. It does this by repeatedly calling the type function, which lets it get the circularly referenced module once the cycle finishes loading.

However, the problem I'm describing here is not with Type-GraphQL's ability to build the schema, but rather the implications on the rest of my code. Because the @Field synchronously calls the type function, it makes it difficult to work with for the rest of my code, since I now have no way to lazily load the circular dependencies to avoid seeing undefined.

I believe patching @Field to ensure that the type function isn't called on the synchronous path should fix this issue. I'm investigating that now, but I'm pretty new to the codebase ...

[MichalLytek]

but rather the implications on the rest of my code. Because the @field synchronously calls the type function, it makes it difficult to work with for the rest of my code, since I now have no way to lazily load the circular dependencies to avoid seeing undefined.

I don't really get what's the problem here. The modules are evaluated once, only the type function is called a two times (first time to check if it returns array, second time to actually get the time). What lazy loading you want to achieve with building server graphql schema? 😕

[davewasmer]

Sorry, some of my terminology got fuzzy here. Let me try to use some code as an example.

Take a typical Post model for a blog:

// models/post.ts
import Comment from './comment.ts';

class Post {

  @Field(_type => [Comment])
  comments: Comment[];

Now, when Node loads this file, it hits the import Comment ... line and loads that file. If the models/comment.ts file (or any of it's dependencies) import models/post.ts, then they'll get an empty object rather than the Post class, since the models/post.ts file hasn't finished executing yet.

The typical way this is solved is by using require and "lazily" loading the dependency. Basically, you just have to make sure the models/post.ts can finish it's initial execution before you attempt to load models/comment.ts:

// models/post.ts
import Comment from './comment.ts'; // this is okay because Typescript will strip this out of the compiled JS since it's not used anywhere except type annotations

class Post {

  @Field(_type => [require('./comment.ts')])
  comments: Comment[];

However - the problem here is that TypeGraphQL actually calls the _type => [require('./comment.ts')] function immediately. Which means we're back to square one - loading models/post.ts will immediately try to load models/comments.ts, which will immediately try to load models/post.ts, which results in Post being an empty object.

The change I've proposed in #237 appears to keep existing functionality the same, but simply delays invoking the _type function, which lets models/post.ts finish loading, and fixes the circular dependency

[MichalLytek]

I know what TypeGraphQL is doing. I wonder what your code is doing beside defining TypeGraphQL classes. What is you use case of module initialization that you suffer from this behavior and dynamic require fix that?

The moment when I call the type getter makes no difference in term of "get an empty object rather than the Post class, since the models/post.ts file hasn't finished executing yet". This is how node resolvers circular module dependencies - one of them must be an empty object first, then it's replaced with actual exports after the depended module initialize.

I just need to wait for the initialization before using (reading) the values, so that's why they are a type getter function in TypeGraphQL and in TypeORM that are called after the modules initialization.

Typescript will strip this out of the compiled JS since it's not used anywhere except type annotations

This sentence is not true because TypeScript use this imported "types" (actually it's a runtime class) to reflect the design types (tslib_1.__metadata("design:type", user_1.User)):

import { User } from "./user";
class Recipe {
  @Field(type => User)
  @ManyToOne(type => User)
  author: User;
}

const user_1 = require("./user");
tslib_1.__decorate([
    src_1.Field(type => user_1.User),
    typeorm_1.ManyToOne(type => user_1.User),
    tslib_1.__metadata("design:type", user_1.User)
], Recipe.prototype, "author", void 0);

[stevefan1999-personal]

Unfortunately, this is a Node problem, but to deal with it is simple: Use a star import on an index file and access all classes under that namespace.

This is to cheat the dependency graph as the namespace served as a global value to all possible class object references. This is also how you workaround cyclic TypeORM entity reference as well. Later when the type loader needs it, it will work because all modules are loaded.

The downside of doing it is that this is very verbose and default import is disallowed.

Other ways of dealing with this could be finding the feedback vertex set but this is a NP-Complete problem.

[davewasmer]

I just need to wait for the initialization before using (reading) the values, so that's why they are a type getter function in TypeGraphQL and in TypeORM that are called after the modules initialization.

Yep! To clarify - for TypeGraphQL's purposes, the current approach works. The problem I'm encountering is in my app code; TypeGraphQL isn't breaking.

But the problem is that, while TypeGraphQL's approach works for TypeGraphQL, it much more difficult to avoid this category of problem in my app code.

This sentence is not true because TypeScript use this imported "types" (actually it's a runtime class) to reflect the design types

Ah, great point! That's what I get for trying the code in the TS Playground first vs. checking my actual output 😉

What is you use case of module initialization that you suffer from this behavior and dynamic require fix that?

I've tried to avoid describing my specific use case since there's lots of complexity that doesn't directly relate to the problem at hand. I'll try to describe it here in the simplest form I can that still reproduces the problem.

The core problem is that I'm trying to model a polymorphic relationship by "denormalizing" it into a series of relationships with possibly null values. This structure creates circular imports.

I have a few entities I'm working with:

• File - represents a generic type of uploaded file from a user
• Video - a user-uploaded video file. Extends File with a few additional video-specific properties
• ProcessingTask - represents the status of a processing task that is performed on an uploaded file

A ProcessingTask can belong any kind of upload, i.e. a File or Video. Rather than modeling this as a traditional polymorphic relationship (i.e. with a type and id column), I just split it out into nullable relationships (i.e. fileId, videoId).

The resulting code looks something like:

// entities/file.ts

import ProcessingTask from './processing-task';

class File {

  @Field(_type => [ProcessingTask])
  processingTasks;

// entities/processing-task.ts

import File from './file';
import Video from './video';

class ProcessingTask {

  @Field(_type => File)
  file;
  @Field(_type => Video)
  video;

// entities/video.ts

import File from './file';

class Video extends File {

If entities/file.ts happens to be imported first, then we see:

• File imports ProcessingTask
• ProcessingTask imports Video
• Video imports File

When Video imports File, it gets an empty object because entities/file.ts hasn't finished its first pass execution yet. But Video needs to extend from File immediately, so it ends up failing.

Hopefully that helps make the situation a little clearer. Happy to jump on Gitter to hash this out in realtime if that's easier too!

---

As a side note: what are there downsides you see to adopting #237? I certainly want to make sure we understand the problem, but there hasn't been any discussion yet of the fix, so I'm curious if there's something I'm missing that makes #237 a more difficult change than I thought.

[davewasmer]

And just to clarify the problem and potential solution from my example:

If we can delay that first step of the import loop, i.e. when File imports ProcessingTask, we can avoid the entire problem.

If File doesn't import ProcessingTask until after it's first pass execution is complete, then Video will be able successfully import File and immediately use it.

Normally the way to accomplish this is by switching from import to require(), and only invoking require() at the moment when you actually need the dependency. In this case, we would replace _type => [ProcessingTask] with _type => [require('./processing-task')].

But TypeGraphQL makes this impossible, because the Field decorator immediately invokes the _type => [require('./processing-task')], resulting in the exact same execution order as when we used import.

My PR (#237) changes the Field decorator so that it waits to invoke the _type => ... function until it's actually needed, meaning that (in my example) entities/file.ts is able to complete it's first-pass execution, and thus Video is able to successfully import File.

[MichalLytek]

what are there downsides you see to adopting #237

It's a dirty hack that rely on mutating the variable in unknown, later time.
The current check for array is also a dirty hack and I will try to avoid it in pipeline redesign (#183).

I'll try to describe it here in the simplest form I can that still reproduces the problem.

I know that circular dependencies in node.js require are problematic but the well-adopted solution by many libs (i.a. mongoose, graphql-js) is to use thunks - functions that returns something, if it rely on other similar deps.

So in graphql-js, you use fields: () => ({ ... }) syntax in ObjectType constructor to lazy reference other graphql types.

I've tried to avoid describing my specific use case since there's lots of complexity that doesn't directly relate to the problem at hand.

I know but I still don't know how the removing of return type function call helps your code because it's still required and evaluated in the same time because of the reflection... also, why you need to lazy call require? Maybe it's a common pattern and I should merge this #237 fix instantly because it may break so many apps 😉

[davewasmer]

It's a dirty hack that rely on mutating the variable in unknown, later time.

I'd suggest that the current code is equally hack-ish, since it rely's on Node's "mutate the object reference once it's ready" behavior. In some sense, my PR would improve the situation by waiting to retrieve the type value until we know something will be there.

it's still required and evaluated in the same time because of the reflection

I ended up typing the processsingTasks field as any to avoid the reflection emit causing an import. Not ideal, of course, but it's the only way I could see to solve this.

the well-adopted solution by many libs (i.a. mongoose, graphql-js) is to use thunks - functions that returns something, if it rely on other similar deps.

Yep, but the key component here is that those other libraries all delay invoking the thunk. If the thunk is immediately invoked, then it loses its value, because the import/require is back on the synchronous execution path.

Using your example of graphql-js's ObjectType constructor, if we look at the source code for the constructor you can see that they don't immediately invoke the thunks - they save the thunks off as _fields and only invoke the thunk later on when it's actually needed.

I still don't know how the removing of return type function call helps

To clarify - I'm not suggesting removing the function call entirely. I'm suggesting that we delay the call until it's absolutely necessary.

it's still required and evaluated in the same time because of the reflection

See above - if you type it as any then it isn't evaluated due to reflection

also, why you need to lazy call require?

My goal with the example above was to explain why the lazy require call is necessary. Is there a part of that example I can expand on that will help make that a bit clearer?

because it may break so many apps

Sorry, a bit confused here since the tests are all passing on #237. Are you suggesting that lots of people may be relying on the internal behaviors of the framework (i.e. somehow their code relies on the return type function being called synchronously) and would be caught off guard by this change?

---

If you're hesitant to make this change, mind if I ask how you'd suggest I get my example scenario working? Or does TypeGraphQL not have plans to support this kind of data model?

I don't mind if it's a bit hacky (obviously, since my solution involves typing as any), but I'm a little stuck without #237 because as far as I can tell this kind of data model is effectively impossible with TypeGraphQL, with or without ugly hacks.

[MichalLytek]

those other libraries all delay invoking the thunk

So do I. The "fixed" part of the code is responsible only for checking for [Foo] syntax, not for getting class reference. Because it might not exist during the decorators evaluating, which is in the middle of module loading.

https://github.com/19majkel94/type-graphql/blob/0b0b1f5ae4a21264225f41a3d317afdd9a769653/src/schema/schema-generator.ts#L258

Or does TypeGraphQL not have plans to support this kind of data model?

What kind of data model? Circular dependencies between object types? It works, it's tested. What else do you need? Show me what you want to achieve, not how to fix the codebase. Then I can show you how to use or make a fix.

[davewasmer]

So do I. The "fixed" part of the code is responsible only for checking for [Foo] syntax, not for getting class reference.

Perhaps I'm misunderstanding, but it seems like you are not delaying the thunk invocation:

1. The @Field decorator immediately calls findType
2. findType immediately calls returnTypeFunc

Am I misusing the @Field decorator?

What kind of data model?

Apologies if my previous comments were unclear. I'll try to restate my example to clarify.

I have the following classes:

• a File class which has many ProcessingTasks
• a Video class that extends that File class
• a ProcessingTask which has one File or Video

That's what I'm trying to achieve - I'd love your help figuring out how to accomplish that with TypeGraphQL. As far as I can tell, it's not currently possible, but I'd love to be wrong! 😉

[MichalLytek]

As far as I can tell, it's not currently possible,

Do you have a repository with reproducible code example? Do you have an error log or description what's happening (other that "not working")? It will be really helpful with detecting the pattern you use and how the thunk delay is helping with it.

[MichalLytek]

I've tried to reproduce your case with File, Video and ProcessingTask.

Three things:

1. Running normal TypeGraphQL code results in this error:

export default class Video extends File {
                                   ^
TypeError: Class extends value undefined is not a constructor or null

2. Running the same code but without TypeGraphQL decorators - everything is ok.
3. Running the same code with decorators and thunk array detection fix - the same error.

I've also tried to disable emitting the metadata but without any luck.
It's just because the imports in the first line that are used in thunks decorators.

[MichalLytek]

Ok, changing the thunk imports to @Field(_type => [require("./processing-task").default]) fixes the error extends value undefined when the fix is applied.

Now I know what the use case is and why we need to fix that 😉
Still would be simpler with simple reproducible code example repo and proper error message.

Closing via #237 🔒