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
New features for GraphQL typescript plugin #1757
Conversation
About refactoringGenerally using all 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) |
@kamilmysliwiec any feedback on this? |
I'll take a look as soon as I can |
Prior to this change typescript transform plugin processed all classes in files with a matching names. This is probably not desirable behaviour because there might be classes not exposed to graphql and they should not be processed This change makes processing a bit smarter, so it only processes classes decorated with ObjectType / InterfaceType and InputType decorators.
2 new features added:
/** Test1 Description */
@ObjectType()
class Test1Model {}
@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 @ObjectType()
class PersonModel {
type: 'truck driver' | 'manager';
}
// will produce:
export type PersonModelTypeEnum {
truck_driver = 'truck driver'
manager = 'manager'
} |
3 more features added:
// previously
registerEnumType(MyAwesomeEnum, {name: 'MyAwesomeEnum' })
// now (name is inhereted from Enum name)
registerEnumType(MyAwesomeEnum)
// previously
const MyAwesomeUnion = createUnionType({name: 'MyAwesomeUnion', types: [Foo, Bar]})
// now (name is inhereted from variable name)
const MyAwesomeUnion = createUnionType({types: [Foo, Bar]})
/** 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
*/
enum Status {
ENABLED,
DISABLED
}
// OR
/**
* @HideEnum
*/
enum Status2 {
ENABLED,
DISABLED
} |
I checked plugin on our codebase found couple issues which fixed in previous two commits and it works without any issue even with |
2a6b2b9
to
08c7c49
Compare
When code is downleveled to ES5 it automatically change all usages of imported members except those which was added by transform. This breaks `registerEnumType()` calls added by transform. The solution is create an explicit dedicated import only for this method in the transform and not relay on exisitng imports at all.
08c7c49
to
e8763d6
Compare
@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. |
…eck something Previous test used transpile-only code, which produced code not showing actual transformation therefore this test didn't actually check that imports correctly added
Nested types introspection was useless because runtime did not understand this code. There is no way to use "anonymous" type literals in graphql schema Add tests for replaceImportPath util, because current test suite didn't cover all branches in it.
Changes since last time:
|
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. |
I meant Linkedin! Apologies
There should be "no minimal version", in theory of course. Practically, everyone should use >= 3.x (and if you use Will respond to other comments as soon as I can! |
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 transformationRight 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 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:
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 What do you think? Does it make any sense? |
Both suggestions sound good to me |
New featurePlugin now can introspect type from getters: @ObjectType()
class ObjectTypeModel {
get prop(): string {
return 'something';
}
} |
Would you be able to rebase your changes @thekip? If you're busy, I can look into this instead |
I will |
…lugin # Conflicts: # .prettierignore # packages/graphql/lib/plugin/utils/ast-utils.ts # packages/graphql/lib/plugin/visitors/model-class.visitor.ts # packages/graphql/tests/plugin/fixtures/nullable.dto.ts # packages/graphql/tests/plugin/model-class-visitor.spec.ts
@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. |
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. |
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. 😞 |
@kamilmysliwiec if some features are controversial, we can drop them and merge only those which are more predictable and can bring more value. |
I synchronized the branch with the latest upstream, but e2e tests still failing with 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. Anyway, is it something actionable from my side? |
I linked forked plugin to our app, and found an issue introduced with this commit 3fe7cdb 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. |
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:
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 |
Ok, i will take care of that |
I went ahead and removed features that I've mentioned here #1757 (comment) Feel free to create separate PRs with those individual features 🙌 Fantastic PR, thanks for your contribution! |
Since this new version I just updated my code with the version, and all field on args disappear from the graphQL schema. Anyone with the same issue ? |
@EdouardBougon could you create a new issue with a minimum reproduction repository? |
@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? |
Doing it right now |
yes I confirm, it fix the issue |
@kamilmysliwiec Do you still need a repo to test the issue ? |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently GraphQL Plugin does not understand
@deprecated
tag in JsDoc comments for fields.Issue Number: #1346
What is the new behavior?
This PR adds support for
@deprecated
tag. It parses it in following fashion: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?
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 #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 :)