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

chore(null): switch strictNullChecks config from opt-in to opt-out for new files #6284

Merged
merged 5 commits into from
Dec 20, 2022

Conversation

dbjorge
Copy link
Contributor

@dbjorge dbjorge commented Dec 20, 2022

Details

This PR updates our strictNullChecks config and tooling to include all non-test .ts and .tsx files in the strict null checks set by default, only excluding those files explicitly marked as having known issues in tsconfig.strictNullChecks.json. This:

  • Simplifies the strictNullChecks config file a lot (it reduces the size by half and is now a flat list of files instead of a mix of files and glob patterns)
  • Saves a step of having to manually run yarn null:autoadd for most changes, since new files are now implicitly added
  • Removes the corresponding item from our PR checklist (I left the automated PR check in place since it doesn't hurt anything and has a self-descriptive error message if it does fail)

To implement this, I wrote a script to perform the conversion from an include list to an exclude list. I included it for review in this PR, but I don't expect it to need to be run again in the future. I verified that yarn null:progress reports finding the same counts of checked and eligible files before and after the change:

Web strict-null progress

81% complete (1273/1562 non-test files)
Contribute at #2869. Last update: Tue Dec 20 2022

I ensured that the assorted helper scripts (yarn null:find, yarn null:autoadd, yarn null:check) all still work as expected.

Motivation

#2869 and see above

Context

I considered renaming autoadd to something else now that it's arguably "removing an exclusion" rather than "adding an inclusion", but I decided to leave it as it; it's still "adding new files to the strict null checked set" and it felt like more work/confusion than it was worth to try to update all the issues/team working knowledge/etc about it.

Pull request checklist

  • Addresses an existing issue: Codebase should use strict null checks #2869
  • [x] Ran yarn null:autoadd
  • Ran yarn fastpass
  • [n/a] Added/updated relevant unit test(s) (and ran yarn test)
  • [n/a] Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). See CONTRIBUTING.md.
  • [n/a] (UI changes only) Added screenshots/GIFs to description above
  • [n/a] (UI changes only) Verified usability with NVDA/JAWS

@dbjorge dbjorge requested a review from a team as a code owner December 20, 2022 18:56

fs.writeFileSync(tsconfigPath, serializedContent);
let prettierConfigPath = path.join(__dirname, '..', '..', 'prettier.config.js');
Copy link
Contributor Author

@dbjorge dbjorge Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this change, yarn null:autoadd would try to write out the include: property on multiple lines and conflict with prettier wanting it on one line

Copy link
Contributor

@madalynrose madalynrose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!!

@dbjorge dbjorge merged commit 1ea9642 into microsoft:main Dec 20, 2022
@dbjorge dbjorge deleted the null-convert-to-exclude branch December 20, 2022 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants