type-graphql icon indicating copy to clipboard operation
type-graphql copied to clipboard

[Docs] Add installation notice for strict mode

Open ecklf opened this issue 4 years ago • 9 comments

Describe the issue The installation guide should add a mention that using strict type-checking requires making class properties with a ! or disabling strict checking for property initialization in classes. Elsewise users will face the following error:

Property 'xy' has no initializer and is not definitely assigned in the constructor

My suggestion (to be added at the end of https://typegraphql.com/docs/installation.html):

In case of using strict type-checking in tsconfig.json, all non-optional class properties are required to use the non-null assertion operator !.

As an alternative solution strict checking of property initialization in classes can be disabled:

"strictPropertyInitialization": false

Are you able to make a PR that fix this? I can do if my suggestion is ok.

ecklf avatar Jun 05 '20 09:06 ecklf

I do not mention strict mode or require it in installation docs.

So if someone decides to use that, I believe it should know about !: syntax or find out about that as Property 'xy' has no initializer and is not definitely assigned in the constructor error is easily searchable.

As it's not TypeGraphQL related, I don't want to pollute the docs with basic TS info as the more not required info it contains, the less people read the docs carefully 😕

MichalLytek avatar Jun 05 '20 09:06 MichalLytek

I don't want to pollute the docs with basic TS info as the more not required info it contains, the less people read the docs carefully

Fair point, closing this then 👍

ecklf avatar Jun 05 '20 09:06 ecklf

@MichalLytek Thank you for a great library!

I'm fully aware what ! means, but I was unsure if it would work with the reflection magic going on in type-graphql. So is this the recommended way? What about providing actual default values?

I think it's really worth mentioning in the docs. Why not just make it easier for new users to use this library? It's not all about users "choosing" strict mode, some frameworks / starter templates use it by default nowadays.

memark avatar Aug 18 '20 16:08 memark

I'm a full-time typescript developer and I couldn't make much sense of this error either. Using 'strict'-mode is kind of the default when writing typescript. I would definitely consider adding a note about it on the 'Getting started'-page.

I'll include the errors I encountered so that search engines find this page better. Hopefully it will help other first-time TypeGraphQL users out.

Property 'id' has no initializer and is not definitely assigned in the constructor.

I'm sure you would like a wide adoption of TypegraphQL and with that assumption I'll share my expectations and thoughts when first trying it out today: I would expect to be able to follow the "Getting started"-flow and not be surprised by mystical errors. Especially not errors caused by sensible (typescript) defaults.

Anyways, it seems manageable enough. Although I'm unsure how I would go about setting fields as required with strict null checks enabled. Nullable fields seem easy enough; A quick read through the docs suggest that we need to set @Field({nullable: true}) if setting the property to nullable with ?.

So, how would I set a field to be non-nullable with strict null checks enabled? If the solution is what @ecklf suggests, then I would probably just set all the fields to be nullable. I don't trust myself, and don't know TypeGraphQL well enough (or at all) to know that a property definitely won't be null. Thus, I'll set them all to nullable so I always check for it.

And now onward through the 'Getting Started" and try to figure out why RecipeService in my Recipie resolver throws an error, or rather, what the hell that type is supposed to be and how to define it..

I humbly suggest updating the 'Getting Started' documentation to something a new user can follow along and always have a working app after each step. As of now, if I copy the full example from the page it gives me lots of errors and I think I would have to be or get familiar with TypeGraphQL to figure them out. Kind of a circular problem; I want to learn TypeGraphQL, so I follow the 'Getting Started'-example, but I have to know TypeGraphQL to get the example working... ♻️

MrEmanuel avatar Jul 28 '21 09:07 MrEmanuel

Just want to point out, I don't know how long this has been the case, however strict type checking is the default in TypeScript now. This means the given "Getting Started" examples are invalid with a default configuration.

I can understand the sentiment of not wanting to clutter documentation with useless info, however the unwillingness to include this type of info makes me nervous to use your library. I am concerned that another issue will pop up in the future that's less obvious of a fix but isn't documented simply because you want to keep the documentation as clean as possible.

Kind of a contradiction of what documentation is intended for, no? If the documentation is short, sure I'll read it, but if it doesn't help me get my application working, then what good is it?!

robere2 avatar May 21 '22 00:05 robere2

I share @robere2 's sentiment here. It's kind of ridiculous if the default strict ts configuration results in ts errors out of the box for this library.

Is the solution to actually set "strictPropertyInitialization": false ? Or what is the solution here?

It's silly to assume devs can just google something when there are many different outcomes that can result in bad practices. I want to support this library since it has good traction in the graphql orm ecosystem, but I think it should come with some seniority in terms of best practices.

alxvallejo avatar Jul 25 '22 15:07 alxvallejo

PR would be greatly appreciated :pray:

MichalLytek avatar Jul 25 '22 16:07 MichalLytek

I'm evaluating this product for my team and have the same concerns, so I'm happy to see this get reopened.

I'd be willing to take a stab at a PR but am stuck on @MrEmanuel 's comment. It seems the options on the table are:

  1. ! everything. With this I lose all code-time type safety around null checks for these properties.
  2. ? everything with nullable: true. With this I have type safety, but have to do a lot of unnecessary null checking. I also have to make client-facing changes to the schema that I don't want to make, and as a result of those changes consumers also have to do the same unnecessary null-checking.
  3. Disable strictPropertyInitialization.

1 and 2 are not viable to me. Other than having to set up a special tsconfig.json to disable that for a subset of files, what are the drawbacks of option 3? It would allow me to be semantically accurate (nullable where fields are nullable, and not where they're not). I would have type safety when defining or passing around objects of this type.

If that's the case, I'd be happy to drop in a PR with @ecklf's original suggestion.

maslade avatar Jul 25 '22 20:07 maslade

Hello, has there been an established convention for handling this error since the last update? Currently disabling strictPropertyInitialization. Has anyone come across any long-term issues from proceeding this way?

weoreference avatar May 06 '23 21:05 weoreference