introspectComments for graphql plugin causes wrong require paths in compiled js code

[dzunftmeister-evorhei]

trafficstars

I'm submitting a...

[x] Regression 
[ ] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

if the introspectComments option for @nestjs/graphql is set to true in nest-cli.json, the compiled src has wrong require paths

Expected behavior

Minimal reproduction of the problem with instructions

clone https://github.com/dzunftmeister-evorhei/bson-issue3 npm install npm run start:dev

What is the motivation / use case for changing the behavior?

Environment

Nest version: 8.0.2

 
For Tooling issues:
- Node version: 14.17.3
- Platform:  Linux

Others:
PM: npm
OS: Ubuntu 18.04

[kamilmysliwiec]

Please provide a minimum reproduction repository. The one you shared contains several resolvers and has a dependency on your GH repositories.

[dzunftmeister-evorhei]

i reduced the complexity in the same repo as far as i can

[timofei-iatsenko]

@dzunftmeister-evorhei
Hi, i was able to reproduce your issue. The root cause is not related to introspectComments but related to GraphQL plugin itself.

@kamilmysliwiec the root cause is in the plugin "eager" import feature. Taking the following input:

import { Field, ID, ObjectType } from '@nestjs/graphql';
import { ObjectId } from 'mongodb';

@ObjectType()
export class Demo {
  @Field(() => ID)
  id!: ObjectId;
}

It produces:

const eager_import_0 = require("bson/bson");
const graphql_1 = require("@nestjs/graphql");
const mongodb_1 = require("mongodb");
let Demo = class Demo {
    static _GRAPHQL_METADATA_FACTORY() {
        return { id: { type: () => require("bson/bson").ObjectID } };
    }
};
__decorate([
    graphql_1.Field(() => graphql_1.ID),
    __metadata("design:type", mongodb_1.ObjectId)
], Demo.prototype, "id", void 0);

Look at the require("bson/bson"); line.

I still don't understand the purpose of this explicit import adding feature. Could you elaborate a bit on that?

[timofei-iatsenko]

Well, now i know more insights about this graphql plugin and understand why we need this "eager" imports. This issue quite hard to tackle, but i have an idea how to workaround this.

I will try to fix this here https://github.com/nestjs/graphql/pull/1757

[koriwi]

sitting here with the same problem. patiently observing...

[timofei-iatsenko]

Pushed a workaround into PR, waiting for maintainers...

[koriwi]

@thekip i dug deep into the code and ended up reverse engineering model-class.visitor.ts without real success. Our problem is that we use a monorepo and the @miralytik/common import (which is a package in our monorepo) gets replaced with a relative path by following the symlink. As you can imagine the path is incorrect after building the backend because the folder structure is different when all files are in dist/ Will the code behave the same in this regard after your changes?

[timofei-iatsenko]

My changes is not a fix but rather a workaround. It allows developer to override default behavior and completely switch off type introspection for one particulare field. With my changes if you set @Field(() => Type) for the field it will not even try to figure out the type and will not produce any imports.

The actual problem lay somewhere in Typescript type checker. We just ask it something "give full type reference" and it returns something like import('/path/to/file').MyType which then plugin convert to relative path, remove node_modules part and etc. I'm not sure that this part of type-checker was especially designed for this use case.

This solution is not ideal in my opinion, and we probably should not use type-checker at all and restrict developers to use only type references which point on real type or primitive.

To illustrate why we use type-checker look at this example:

// file a.ts

export class ModelA {}

export type ModelAlias =  ModelA;

// file b.ts
import {ModelAlias} from '/a.ts';

@ObjectType()
export class ModelB {
 field: ModelAlias;
}

Will resolve type alias, thanks to type-checker and produce a valid import in downleveled code:

// ModelAlias -> ModelA
require(/a.ts).ModelA

[timofei-iatsenko]

I also worked on the type-checker free version in another branch, but decided to delay it for a while, because there are so many cases to tackle and impact on community would be very big if release it as it is

[timofei-iatsenko]

@koriwi By the way, could you prepare a mimimal repro copying your monorepo structure, i will look, may be we can fix it differently.

[koriwi]

@thekip i can. it will take a while because i'm new to nestjs. But i can give you a short overview before creating the repo.

npm 7 monorepo with workspaces:

• package.json (with workspace information)
• packages
  • backend
    • node_modules
      • npm symlink to common/dist/index (automatically handled by npm and works correctly with normal tsc)
    • src
      • some_code.entity.ts which does import {broken_enum} from '@common' <- this gets transpiled from @nestjs/graphql to '../../common/enum'. this wont work from the dist folder because we need one more ../ there. when i manually change it to @common it works as expected
    • migrations
    • dist
      • src
      • migrations
  • common
    • dist
    • some types
    • enum type which is not completely emitted after compiling (this enum is wrongly imported)

[dan-cooke]

I also experienced this crazy bug.

Although your report of introspectComments causing the issue is a red herring I think.

I believe the issue is related to the typeFileNameSuffix actually. I only encouter the error in my files named *.entity.ts

If I remove the .entity.ts suffix, and use @Field() manually, i can get around this error.

I lost 3 days debugging this one, really really confusing -

[timofei-iatsenko]

When you rename your file and deleted suffix .entity.ts you just excluded this file from typescript plugin transformation. The effect is the same as just disable this plugin at all, but only for one file. So the issue is not related to typeFileNameSuffix neither to introspectComments. It's just a sideffect of some unlucky type resolving inside the plugin which may cause issues in some cases. The new version of plugin may solve the issue. It allows overriding on a field level, no need to exclude the whole file from processing. Just add @Field for problem place and issue should be solved.