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

Support overriding type-checking compile flags on a per file basis #8405

Closed
robertknight opened this issue Apr 30, 2016 · 15 comments
Closed
Labels
Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds

Comments

@robertknight
Copy link

I would like to make use of the --strictNullChecks option in an existing TypeScript codebase that is ~25K LOC in size. Not surprisingly, this throws up a substantial number of errors (~640).

It would be very helpful if it was possible to tackle them gradually and keep the codebase compilable the whole time. The need to be able to tackle these issues gradually is especially true for --strictNullChecks because a reasonable number of the errors that came up are real potential issues that require some thought, rather than problems which can just be fixed in a mechanical fashion.

Ideally what I would like is to be able to suppress checks on a per-file basis, enabling a gradual conversion as follows:

  1. Enable --strictNullChecks for the project
  2. Attempt to compile and add // @typescript:-strictNullChecks to each file that produces an error
  3. Pick the first file with such a directive, remove it and fix any errors.
  4. Repeat step 3 for each file with such a directive

Although it is possible to suppress errors with the ! operator, this could end up hiding real bugs and it would be unclear when the operator was used in the process of migration vs. to suppress an error in a case where it isn't possible to statically prove to the compiler that a value is non-null.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 30, 2016

We did talk about this suggestion before. specifically in the context of declaration files. the conclusion was that it is too subtle of a gesture to change the behavior of the type system.
I will bring it up again for discussion.

Would breaking the project into two pieces (e.g. some modules), and building each separately be an option?

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Apr 30, 2016
@robertknight
Copy link
Author

robertknight commented May 1, 2016

We did talk about this suggestion before. specifically in the context of declaration files.
the conclusion was that it is too subtle of a gesture to change the behavior of the type system.

Ah, presumably in declaration files that was about changing the behavior of the type system at the point of declaration where it would have non-local effects. Here I'm looking to change the behavior at the point of use. If there was an exported interface declared in a file with a // @typescript:-strictNullChecks directive, I think it would make sense that the directive not affect consumers of the interface in other files.

Would breaking the project into two pieces (e.g. some modules), and building
each separately be an option?

Yes, although that is quite a lot of hassle compared to per-file or directory directives. An alternative that came to mind would be removing the --strictNullChecks option in the normal compile and instead writing a separate tool using the compiler API which compiled the same project using stricter flags and then filtered out the diagnostic messages based on directives in the file or some other config mechanism. Once the entire codebase had been converted over, the option could be turned on in the normal build.

@mhegazy
Copy link
Contributor

mhegazy commented May 17, 2016

This looks like a point-in-time migration request. once all --strictNullChecks issues are handled there will be no need for the per-file flag. on the other hand it is not clear what the flag would mean, suppress errors or change the meaning of T to be T | null | undefined, if it is the first, then these types could leak, and even if you are not getting the errors you are still getting their effect, which is inconsistent at best; if it is the second, the compiler does not have machinery to do this today, so there is a lot of plumping that needs to be done to achieve this, and even with that, the meaning of a type will depend on the header of a file, which is confusing. Moreover any change to the behavior would be a forking of the language. So the complexity introduced in the compiler and to the user we do not believe this is something we should pursue.
Please see #7140 (comment) for more details.

@mhegazy mhegazy closed this as completed May 17, 2016
@mhegazy mhegazy added Too Complex An issue which adding support for may be too complex for the value it adds and removed In Discussion Not yet reached consensus labels May 17, 2016
@OliverJAsh
Copy link
Contributor

@robertknight

An alternative that came to mind would be removing the --strictNullChecks option in the normal compile and instead writing a separate tool using the compiler API which compiled the same project using stricter flags and then filtered out the diagnostic messages based on directives in the file or some other config mechanism.

I built something along those lines in #8855 (comment). However, the issue I have now is getting IDEs to provide corresponding feedback to developers. Presumably you would need to wrap the IDE plugin to do some filter, somehow? I opened microsoft/vscode#10337 to try and get more information on how to do this.

@pcj
Copy link

pcj commented Dec 13, 2016

Closure compiler has the per-file @Suppress {compilerCheck1|compilerCheck2|...} which is quite useful. Would be good to have this in typescript.

@evmar
Copy link
Contributor

evmar commented Feb 22, 2017

Could you do something like:

  1. enable --strictNullChecks locally
  2. fix some of the produced errors
  3. undo step 1
  4. submit the change from number 2
  5. go back to step 1 again

In a loop until the whole project passes? Note that it's safe to check in strict null annotations on a project without SNC enabled.

@TKharaishvili
Copy link

@evmar Migrating the project to strict null checks is the kind of a change that people will probably want to do from time to time, gradually when they have no other pressing tasks.

If one person updates a couple of files to conform to strict null checking and switches the --strictNullChecks option back off, other people on the team may unknowingly or by accident reintroduce unchecked code in the same files, especially when a project is under active development.

I think introducing the gradual strict null checking would take many older projects a long way towards the right direction and would be instrumental in some day switching the --strictNullChecks option on by default(which is how it ideally should be).

@meldridon
Copy link

Adding my voice. We have a legacy project that we want to migrate slowly to strict checking. There is no way we can convert the whole project in one go.

We really want to enable/disable the strict checks on individual files until the migration is complete for the same reason TKharaishvili mentioned: not everyone is going to embrace this cycle of turning on and off strict checking and may inadvertently add code that violates strict code on files that have already been migrated.

@vojto
Copy link

vojto commented Jun 20, 2018

Our solution was to have two TypeScript configs:

  1. For IDE (VSCode), enable strictNullChecks
  2. For WebPack, disable

Workflow looks like this:

  1. Edit files in IDE, notice strict-null errors in old files, fix them if easy
  2. Don't introduce strict-null errors for any new files
  3. Webpack running in command line shows the "real" errors

In IDE, we're using the standard tsconfig.json file.

For WebPack, we created a new config file tsconfig.webpack.json, and instruct WebPack to use it with configFileName option in awesome-typescript-loader.

@an5rag
Copy link

an5rag commented Jun 7, 2019

+1 Definitely useful since our project has a legacy folder that we don't want to spend time fixing, but had to disable strict null checking in the entire project because of it.
Is it easier/more-sensible to add the inverse feature? That is, disable strict checking on a per-file basis.

@mockdeep
Copy link

Would love to see this as well. With the --noImplicitAny flag we have more than 6k errors reported. It's going to take years to get that down to size. It would be nice if we could enforce better standards on new code and more systematically target areas of the codebase for improvement. Sifting through that many errors is a huge pain.

if it is the first, then these types could leak, and even if you are not getting the errors you are still getting their effect, which is inconsistent at best

@mhegazy this is still better than the alternative. If it's between no checking vs checking that might miss some cases, I'll take the latter.

In the meantime, I think I'm going to poke around instead with having two ESLint configurations, one that is applied to all files and one that only applies to files that are changed. At least that way we can enforce type signatures in changed code using typescript-eslint.

@evmar
Copy link
Contributor

evmar commented May 10, 2020

@mockdeep Not to distract from the larger issue, but one technique I've used (not for exactly this problem, but hopefully you get the idea) is to do one up-front pass that inserts x: TemporaryAny at every location where there is currently an implicit any, so you can then turn on the check everywhere, and then grep for TemporaryAny in your code when you want to clean up the old stuff.

@mockdeep
Copy link

@evmar thanks for the tip. I think a variation on that I might try is to set them to any up front and then use the no-explicit-any ESLint rule to slowly weed them out.

@johnryan
Copy link

johnryan commented Feb 1, 2021

Interesting article on how Figma did this on a large codebase: https://www.figma.com/blog/inside-figma-a-case-study-on-strict-null-checks/

tldr;
The core of this approach is to compile the codebase twice: once for all files without strict null checks enabled, and once for a list of files that are known to compile successfully with strict null checks enabled.

@rkofman
Copy link

rkofman commented Feb 7, 2024

I know I'm quite late to this conversation, but in case new folks are stumbling onto this issue and are interested in a workaround: https://github.com/allegro/typescript-strict-plugin seems to be a plugin that addresses this issue head on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds
Projects
None yet
Development

No branches or pull requests