definitions with option skipResolverArgs: true makes resolvers optional in d.ts

vgandzyuck commented 10 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current behavior

For the NestJS/GraphQL module, you can configure definitions to automatically create a .d.ts file. So, if you have any resolvers in your application that accept parameters, the definition of these resolvers in the .d.ts file becomes optional.

Minimum reproduction code

https://codesandbox.io/p/sandbox/wild-architecture-jz3y27?file=%2Fschema.d.ts%3A18%2C2

Steps to reproduce

See sandbox

npm start:dev

Expected behavior

Resolvers with/within arguments are not optional in .d.ts

export interface IMutation {
    signUp: User;
    signIn: User;
}

Package

[ ] I don't know. Or some 3rd-party package
[ ] code>@nestjs/common</code
[ ] code>@nestjs/core</code
[ ] code>@nestjs/microservices</code
[ ] code>@nestjs/platform-express</code
[ ] code>@nestjs/platform-fastify</code
[ ] code>@nestjs/platform-socket.io</code
[ ] code>@nestjs/platform-ws</code
[ ] code>@nestjs/testing</code
[ ] code>@nestjs/websockets</code
[X] Other (see below)

Other package

@nestjs/graphql

NestJS version

10.2.8

Packages versions

{
  "name": "",
  "version": "0.0.1",
  "description": "",
  "author": "",
  "private": true,
  "license": "UNLICENSED",
  "scripts": {
    "build": "nest build",
    "format": "prettier --write \"src/**/*.ts\" \"test/**/*.ts\"",
    "start": "nest start",
    "start:dev": "nest start --watch",
    "start:debug": "nest start --debug --watch",
    "start:prod": "node dist/main",
    "lint": "eslint \"{src,apps,libs,test}/**/*.ts\" --fix",
    "test": "jest",
    "test:watch": "jest --watch",
    "test:cov": "jest --coverage",
    "test:debug": "node --inspect-brk -r tsconfig-paths/register -r ts-node/register node_modules/.bin/jest --runInBand",
    "test:e2e": "jest --config ./test/jest-e2e.json"
  },
  "dependencies": {
    "@apollo/server": "^4.9.5",
    "@nestjs/apollo": "^12.0.10",
    "@nestjs/common": "^10.0.0",
    "@nestjs/core": "^10.0.0",
    "@nestjs/graphql": "^12.0.10",
    "@nestjs/jwt": "^10.2.0",
    "@nestjs/passport": "^10.0.2",
    "@nestjs/platform-express": "^10.0.0",
    "@nestjs/typeorm": "^10.0.0",
    "bcrypt": "^5.1.1",
    "cookie-parser": "^1.4.6",
    "dotenv": "^16.3.1",
    "graphql": "^16.8.1",
    "helmet": "^7.1.0",
    "passport-jwt": "^4.0.1",
    "pg": "^8.11.3",
    "reflect-metadata": "^0.1.13",
    "rxjs": "^7.8.1",
    "ts-morph": "^20.0.0",
    "typeorm": "^0.3.17"
  },
  "devDependencies": {
    "@nestjs/cli": "^10.0.0",
    "@nestjs/schematics": "^10.0.0",
    "@nestjs/testing": "^10.0.0",
    "@types/bcrypt": "^5.0.2",
    "@types/cookie-parser": "^1.4.6",
    "@types/express": "^4.17.17",
    "@types/jest": "^29.5.2",
    "@types/node": "^20.3.1",
    "@types/passport-jwt": "^3.0.13",
    "@types/supertest": "^2.0.12",
    "@typescript-eslint/eslint-plugin": "^6.0.0",
    "@typescript-eslint/parser": "^6.0.0",
    "eslint": "^8.42.0",
    "eslint-config-prettier": "^9.0.0",
    "eslint-plugin-import": "^2.29.0",
    "eslint-plugin-prettier": "^5.0.0",
    "eslint-plugin-unused-imports": "^3.0.0",
    "jest": "^29.5.0",
    "prettier": "^3.0.0",
    "source-map-support": "^0.5.21",
    "supertest": "^6.3.3",
    "ts-jest": "^29.1.0",
    "ts-loader": "^9.4.3",
    "ts-node": "^10.9.1",
    "tsconfig-paths": "^4.2.0",
    "typescript": "^5.1.3"
  },
  "jest": {
    "moduleFileExtensions": [
      "js",
      "json",
      "ts"
    ],
    "rootDir": "src",
    "testRegex": ".*\\.spec\\.ts$",
    "transform": {
      "^.+\\.(t|j)s$": "ts-jest"
    },
    "collectCoverageFrom": [
      "**/*.(t|j)s"
    ],
    "coverageDirectory": "../coverage",
    "testEnvironment": "node"
  }
}

Node.js version

20.9.0

In which operating systems have you tested?

[X] macOS
[ ] Windows
[ ] Linux

Other

No response

kamilmysliwiec commented 10 months ago

Are resolves required when skipResolverArgs is set to false?

vgandzyuck commented 10 months ago

skipResolverArgs: false

export interface IMutation {
    signIn(login: string, password: string): User | Promise<User>;
    signUp(): User | Promise<User>;
}

So yes they are required

kamilmysliwiec commented 10 months ago

Would you like to create a PR for this issue?

vgandzyuck commented 10 months ago

Would you like to create a PR for this issue?

"I will have the opportunity to delve into this only next weekend; unfortunately, I haven't had the chance to be a contributor to NestJS before."

vgandzyuck commented 10 months ago

@kamilmysliwiec I found the issue, can some just create PR according to the rules?

File: packegs/graphql/graphql-ast.explorer.ts Method: toPropertyDeclarationStructure Line: 317

Current code

return {
      name: propertyName,
      type: this.addSymbolIfRoot(type),
      hasQuestionToken:
        !required || (item as FieldDefinitionNode).arguments?.length > 0,
    };

I think that the correct way is not to look on arguments array at all

return {
      name: propertyName,
      type: this.addSymbolIfRoot(type),
      hasQuestionToken: !required,
};

nestjs / graphql