Skip to content
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

Switch to strictNullChecks: true for new projects #1934

Closed
1 task done
dmint789 opened this issue Oct 20, 2024 · 13 comments
Closed
1 task done

Switch to strictNullChecks: true for new projects #1934

dmint789 opened this issue Oct 20, 2024 · 13 comments

Comments

@dmint789
Copy link

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe it

I have realized that Nest JS sets up projects with strictNullChecks: false in the tsconfig.json by default. I was a bit of a beginner in web development when I first started using Nest JS, but I now see that this is not a good setting to use, because it leads to many bugs being missed. Now I have to migrate quite a big code base to strictNullChecks: true, which will cost me a lot of time.

Describe the solution you'd like

This option should not be set in new Nest JS projects (meaning Typescript will treat it as true, since Nest JS sets strict: true for new projects).

Teachability, documentation, adoption, migration strategy

This doesn't require developers having to relearn anything, because it would only apply to new Nest JS projects.

What is the motivation / use case for changing the behavior?

This will prevent developers from having to migrate their code bases if they decided to change this setting in the future. If they wanted to set it to false, they could do so without having to change anything in the code, because that would be more permissive.

@micalevisk
Copy link
Member

Isn't --strict flag on nest new + a mention in the docs enough?

I like the strict mode tho. And at work we had the exact same problem of yours.

@dmint789
Copy link
Author

Isn't --strict flag on nest new + a mention in the docs enough?

I like the strict mode tho. And at work we had the exact same problem of yours.

I think you may have misunderstood. The default does currently use strict: true, but strictNullChecks is set to false by default. I think the power of defaults is strong, and strictNullChecks should just be left undefined (meaning Typescript would treat it as true due to the strict: true value).

@KostyaTretyak
Copy link

@dmint789, starting with ES2022, the strictNullChecks: true option is no longer relevant, since it is no longer necessary to initialize class properties in the constructor.

@dmint789
Copy link
Author

@dmint789, starting with ES2022, the strictNullChecks: true option is no longer relevant, since it is no longer necessary to initialize class properties in the constructor.

But that's not the only thing it checks. The most important thing is that it'll show an error when you do object.key if object could be undefined or null. Not having this check leads to many easily avoidable bugs. I actually just found one that would've been caught by this (I found it after I created the issue).

@micalevisk
Copy link
Member

micalevisk commented Oct 21, 2024

@dmint789

The default does currently use strict: true

I didn't get this one. Using nest new does not comes with strict true mode. And nest new --strict will not enable strict but only the following flags -- and this is expected:

    "skipLibCheck": true,
    "strictNullChecks": true,
    "noImplicitAny": true,
    "strictBindCallApply": true,
    "forceConsistentCasingInFileNames": true,

@KostyaTretyak
Copy link

@dmint789, My apologies, I confused strictNullChecks with strictPropertyInitialization.

@dmint789
Copy link
Author

@dmint789

The default does currently use strict: true

I didn't get this one. Using nest new does not comes with strict true mode. And nest new --strict will not enable strict but only the following flags -- and this is expected:

    "skipLibCheck": true,
    "strictNullChecks": true,
    "noImplicitAny": true,
    "strictBindCallApply": true,
    "forceConsistentCasingInFileNames": true,

Oh, looks like I was mistaken. Then the thing that should be changed is that Nest JS comes with strict mode by default and includes a non-strict flag instead. There's no way that the standard way to use Typescript should be to allow devs to ignore types.

@micalevisk
Copy link
Member

micalevisk commented Oct 21, 2024

Although I like the strict mode, I don't think that we will make --strict the default anytime soon. See nestjs/typescript-starter#192 (comment)

But let's see if Kamil changed his mind on this

@dmint789
Copy link
Author

Although I like the strict mode, I don't think that we will make --strict the default anytime soon. See nestjs/typescript-starter#192 (comment)

I just read that, but I still think the downside of inexperienced devs missing this and way down the line having to migrate a huge codebase to strict mode is a lot higher than the issue of people who are already migrating from JS having to do extra checks. The current solution is like the worst of both worlds: JS devs still have to go through the pain of migrating to TS, and TS devs who started with Nest JS + TS to begin with still have to migrate to strict mode when they realize that it's a better experience.

@kamilmysliwiec please consider if strict mode can be made the default with an optional --non-strict mode.

@gterras
Copy link

gterras commented Oct 23, 2024

I just read that, but I still think the downside of inexperienced devs missing this and way down the line having to migrate a huge codebase to strict mode is a lot higher than the issue of people who are already migrating from JS having to do extra checks. The current solution is like the worst of both worlds: JS devs still have to go through the pain of migrating to TS, and TS devs who started with Nest JS + TS to begin with still have to migrate to strict mode when they realize that it's a better experience.

I think these flags are singular in the way that people will either really need/want them or really don't want them, there is no in-between.

I agree that defaulting to full strict mode is intrusive/overkill for the latter, although the former could benefit from making an informed choice during cli init, since flags like strictNullChecks are likely to generate a lot of errors when turned on in any non-trivial app.

@dmint789
Copy link
Author

I just read that, but I still think the downside of inexperienced devs missing this and way down the line having to migrate a huge codebase to strict mode is a lot higher than the issue of people who are already migrating from JS having to do extra checks. The current solution is like the worst of both worlds: JS devs still have to go through the pain of migrating to TS, and TS devs who started with Nest JS + TS to begin with still have to migrate to strict mode when they realize that it's a better experience.

I think these flags are singular in the way that people will either really need/want them or really don't want them, there is no in-between.

I agree that defaulting to full strict mode is intrusive/overkill for the latter, although the former could benefit from making an informed choice during cli init, since flags like strictNullChecks are likely to generate a lot of errors when turned on in any non-trivial app.

But then we're prioritizing people migrating to Nest JS over new users. Why not make the setup aimed at new users (starting with Nest JS for their projects to begin with) by default? If people migrating want to change the settings, they can. Type safety should be opt-out, not opt-in.

@micalevisk
Copy link
Member

I don't know. I still feel that the only flags that should be turned on by default are: strictNullChecks and forceConsistentCasingInFileNames. They will help JS users to write better code while not fully strict. But due to how --strict flag works, this is impossible to have.

@kamilmysliwiec kamilmysliwiec transferred this issue from nestjs/nest Nov 12, 2024
@kamilmysliwiec
Copy link
Member

#1945

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants