New features for GraphQL typescript plugin

[timofei-iatsenko]

trafficstars

PR Checklist

Please check if your PR fulfills the following requirements:

• [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• [x] Tests for the changes have been added (for bug fixes / features)
• [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Currently GraphQL Plugin does not understand @deprecated tag in JsDoc comments for fields.

Issue Number: https://github.com/nestjs/graphql/issues/1346

What is the new behavior?

This PR adds support for @deprecated tag. It parses it in following fashion:

1. If just tag is presented, with no message then 'deprecated' default message would be used
2. If user specify a deprecation message after a tag:

  /**
  * @deprecated consult docs for better alternative!
  */
  breed: string;

Message would be used.

I'm using built-in typescript jsdoc support (may be need to check from what version typescript started support it) to parse jsdoc and get information of tags.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

There are no general breaking changes, but schema might be fulfilled with unintended deprecation messages, and this could be quite surprising for some.

Other information

This PR is rethinking of https://github.com/nestjs/graphql/pull/1751 Which is affecting bigger scope and might introduce breaking changes. I decided to roll out features one by one in small PR's instead.

Stay updated, i have many new useful features in my plan :)

[timofei-iatsenko]

About refactoring

Generally using all ts.createXYZ method of the compiler is deprecated. We should use corresponding methods from NodeFactory passed as context to transformation eq 'ctx.factory.createXYZ'

Some of the methods has 1 to 1 replacement, some of them have slightly changed name and few no more exists (createLiteral(string | bolean | number), createMutableClone, and etc)

[timofei-iatsenko]

@kamilmysliwiec any feedback on this?

[kamilmysliwiec]

I'll take a look as soon as I can

[timofei-iatsenko]

2 new features added:

1. Plugin now supports extracting description on a type level. Example:

/** Test1 Description */
@ObjectType()
class Test1Model {}

2. Plugin automatically creates "implicit" enums for inline string unions:

@ObjectType()
class PersonModel {
  type: 'driver' | 'manager' | 'client'; 
}

// equivalent of 

export type PersonModelTypeEnum {
 driver = 'driver'
 manager = 'manager'
 client = 'client'
}

registerEnumType(PersonModelTypeEnum, {name: 'PersonModelTypeEnum'});
 
@ObjectType()
class PersonModel {
  type: PersonModelTypeEnum; 
}

This is drastically reduce a boilerplate in cases where full-featured enum is not needed.

The name for enum created as ObjectTypeName + capitalize(fieldName) + Enum. Because GraphQL doesn't support spaces in enums, it replace it with underscore symbol:

@ObjectType()
class PersonModel {
  type: 'truck driver' | 'manager'; 
}

// will produce: 
export type PersonModelTypeEnum {
 truck_driver = 'truck driver'
 manager = 'manager'
}

[timofei-iatsenko]

3 more features added:

1. Auto discover name in registerEnumType()

// previously
registerEnumType(MyAwesomeEnum, {name: 'MyAwesomeEnum' })

// now (name is inhereted from Enum name)
registerEnumType(MyAwesomeEnum)

2. Auto discover name for createUnionType()

// previously
const MyAwesomeUnion = createUnionType({name: 'MyAwesomeUnion', types: [Foo, Bar]})

// now (name is inhereted from variable name)
const MyAwesomeUnion = createUnionType({types: [Foo, Bar]})

3. Auto discover enums (disabled by default). With this feature enabled all enums in processed files would be automatically exposed to graphql schema (registered with registerEnumType). All comments introspection features such as description and deprecation works for enums as well.

/** This enum represents possible statuses */
enum Status {
  /** This is status by default */
  ENABLED,
  DISABLED,
  /** @deprecated please use other options */
  OTHER,
}

// This will produce under the hood: 

registerEnumType(Status, {
  name: 'Status',
  description: 'This enum represents possible statuses',
  valuesMap: {
    ENABLED: {
      description: 'This is status by default'
    },
    OTHER: {
      deprecationReason: 'please use other options',
    }
  }
})

If you want to exclude particalure enum from exposing you need to add @private or @HideEnum jsDoc tag (the tags TBD).

/**
* @private
*/
enum Status {
    ENABLED,
    DISABLED
}
// OR

/**
* @HideEnum
*/
enum Status2 {
    ENABLED,
    DISABLED
}

[timofei-iatsenko]

I checked plugin on our codebase found couple issues which fixed in previous two commits and it works without any issue even with { autoRegisterEnums: true }. Duplicated calls of registerEnumTypes does not brake a runtime.

[timofei-iatsenko]

@kamilmysliwiec please take a look at this, i need a feedback. Am i moving in the right direction and is maintainers interested in such kind of changes in the project.

[timofei-iatsenko]

Changes since last time:

1. Added more tests, and actually check "import management" when code is downleveled. Previously test used transpileModule which processes files in isolation one by one and not taking into account relation between them and therefore not covering "import management" at all.

2. Improved heuristic of fields metadata gathering and now it will not even try to get a type of a field if user specified type in a decorator manually. This a) make execution slightly faster as we not need to execute additional checks, b) allow users to override behavior of a plugin if it's working incorrectly in some reason see #1643

[kamilmysliwiec]

This is looking great so far @thekip, thank you! I left a few minor comments so far but I'll definitely pull all changes and walk through them more meticulously very soon.

Side topic: I'd love to have a chat with you on LN or elsewhere!

[timofei-iatsenko]

Do nestjs still support typescript version prior 4.2? Where i can find a minimal system requirements?

[timofei-iatsenko]

Side topic: I'd love to have a chat with you on LN or elsewhere!

What is LN? 😅 We can actually plan a quick call, where i can explain how everethyng works and what thoughts and plans i have related to this.

[kamilmysliwiec]

What is LN? 😅

I meant Linkedin! Apologies

Do nestjs still support typescript version prior 4.2? Where i can find a minimal system requirements?

There should be "no minimal version", in theory of course. Practically, everyone should use >= 3.x (and if you use @nestjs/config then you need >= 4.1 as it relies on the Template Literal Types feature)

Will respond to other comments as soon as I can!

[timofei-iatsenko]

Well, for such things as a typescript transformation plugin we should define a "minimal version" because we a heavily relay on typescript compiler API which changes over time. So at least for the plugin we should define it and state it clearly in the docs (something like if you want to use typescript plugin you should have typescript > 4.2)

It also would be nice to have to setup test matrix with different typescript versions on different OS (i saw branches in source code which relay on different path windows/unix).

Side topic Error handling in transformation

Right now there are no such thing as error handling. If a transformation could not figure out a type for property it's just silently leave () => Object instead of type. This doesn't have any sense for runtime consumer and it shows error while trying to generate a scheme. Usually errors from scheme generator is quite inaccurate and may point user in wrong direction.

During compiling we have more contextual information (for example inline type literal is used) and we can generate more accurate errors which would point to exact place in source file.

Example:

GraphqlPluginError: Property test in CatDto class has unsupported type (TypeLiteralExpression). __ additional message helping to solve the issue __

The main problem here is how to throw this exceptions. We could not just throw it from the transformation because it will exit a typescript compilation. (not a desirable behaviour in watch mode). Any way need to investigate on topic "How to properly report errors from typesctipt transform"

For me obvious option is generate throw new Error(...) and insert it into sourcecode and throwing errors from the runtime. (errors should be inserted in top level of a module, to be executed as early as possible)

What do you think? Does it make any sense?

[kamilmysliwiec]

Well, for such things as a typescript transformation plugin we should define a "minimal version" because we a heavily relay on typescript compiler API which changes over time. So at least for the plugin we should define it and state it clearly in the docs (something like if you want to use typescript plugin you should have typescript > 4.2)

For me obvious option is generate throw new Error(...) and insert it into sourcecode and throwing errors from the runtime. (errors should be inserted in top level of a module, to be executed as early as possible)

Both suggestions sound good to me

[timofei-iatsenko]

New feature

Plugin now can introspect type from getters:

@ObjectType()
class ObjectTypeModel {
  get prop(): string {
    return 'something';
  }
}

[kamilmysliwiec]

Would you be able to rebase your changes @thekip? If you're busy, I can look into this instead

[timofei-iatsenko]

I will

[timofei-iatsenko]

@kamilmysliwiec i synchonized the branch with current master. Unfortunately i was not able to rebase and keep branch clean, due to file structure changes and re-formatting files. I hope i didn't miss anything.

Please take a look, and let's discuss future of the branch and typescript plugin overall.

[timofei-iatsenko]

I also have quite interesting and 'brave' ideas how to improve DX with code-first approach avoiding sharp edges with typescript compiler and etc. I would like to share it with you because scope of changes might be quite big.

[ggagosh]

Hey @kamilmysliwiec is there any chance that this request merged near future? Without this, the plugin only works with primitive types and can't use ObjectId for example. 😞

[timofei-iatsenko]

@kamilmysliwiec if some features are controversial, we can drop them and merge only those which are more predictable and can bring more value.

[timofei-iatsenko]

I synchronized the branch with the latest upstream, but e2e tests still failing with Killed 137 reason. Quick googling pointing that this is most likely out of memory on CircleCi https://github.com/PetroFit/petrofit/issues/63 (locally all tests passed just fine)

I have no idea what changes exactly causing this out of memory error. I've checked the code and failing package (packages/apollo) even not using typescript plugin at all, so my changes shouldn't cause any issue with these tests. Or probably it's because they run in parallel and together consume more memory than before and apollo package just have a bad luck.

Anyway, is it something actionable from my side?

[timofei-iatsenko]

I linked forked plugin to our app, and found an issue introduced with this commit https://github.com/nestjs/graphql/pull/1757/commits/3fe7cdbdf1118a105f44df44fde7329f55817d38

Looks like something was changed in the mechanism of merging factories provided from the cli plugin and runtime decorators. Latest commit fixes the issue. Now it works on our moderate-sized codebase.

[kamilmysliwiec]

I'd love to merge this PR but I feel like we're trying to bring too many features at once. If we could cherry-pick the following changes for now:

1. Support for the @deprecated tag
2. Migration to use ctx.factory instead of ts.[..]
3. Support extracting description on a type level

that would be fantastic! Afterward, we could think about changes around enums/unions (preferably in separate PRs - for example - auto-generating implicit enums for inline string unions sounds very useful as well and I'd love to get this one released under a flag for now). I understand that this (reverting other changes) might be somewhat time-consuming so let me know if you're willing to work on this one and I can try to cherry-pick the rest by myself! @thekip

[timofei-iatsenko]

Ok, i will take care of that

[kamilmysliwiec]

I went ahead and removed features that I've mentioned here https://github.com/nestjs/graphql/pull/1757#issuecomment-1228416518 Feel free to create separate PRs with those individual features 🙌

Fantastic PR, thanks for your contribution!

[EdouardBougon]

Since this new version "@nestjs/graphql": "10.1.1", it seems that the graphQL plugin doesn't introspect anymore the fields on Args class.

I just updated my code with the version, and all field on args disappear from the graphQL schema.

Anyone with the same issue ?

[kamilmysliwiec]

@EdouardBougon could you create a new issue with a minimum reproduction repository?

[kamilmysliwiec]

@EdouardBougon this line likely causes this issue https://github.com/nestjs/graphql/blob/master/packages/graphql/lib/plugin/visitors/model-class.visitor.ts#L33

Can you modify this array locally (adding ArgsType) and check if it fixes your issue?

[EdouardBougon]

Doing it right now