-
Notifications
You must be signed in to change notification settings - Fork 382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
introspectComments for graphql plugin causes wrong require paths in compiled js code #1643
Comments
Please provide a minimum reproduction repository. The one you shared contains several resolvers and has a dependency on your GH repositories. |
i reduced the complexity in the same repo as far as i can |
@dzunftmeister-evorhei @kamilmysliwiec the root cause is in the plugin "eager" import feature. Taking the following input:
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 I still don't understand the purpose of this explicit import adding feature. Could you elaborate a bit on that? |
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 #1757 |
sitting here with the same problem. patiently observing... |
Pushed a workaround into PR, waiting for maintainers... |
@thekip i dug deep into the code and ended up reverse engineering model-class.visitor.ts without real success. |
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 The actual problem lay somewhere in Typescript type checker. We just ask it something "give full type reference" and it returns something like 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 |
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 |
@koriwi By the way, could you prepare a mimimal repro copying your monorepo structure, i will look, may be we can fix it differently. |
@thekip npm 7 monorepo with workspaces:
|
I also experienced this crazy bug. Although your report of I believe the issue is related to the If I remove the I lost 3 days debugging this one, really really confusing - |
When you rename your file and deleted suffix |
I'm submitting a...
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
The text was updated successfully, but these errors were encountered: