Skip to content

Commit

Permalink
chore(null): switch strictNullChecks config from opt-in to opt-out fo…
Browse files Browse the repository at this point in the history
…r new files (#6284)

#### 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](#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
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [x] Addresses an existing issue: #2869
- ~~[x] Ran `yarn null:autoadd`~~
- [x] 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`
- [x] 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
  • Loading branch information
dbjorge authored Dec 20, 2022
1 parent d6eff3a commit 1ea9642
Show file tree
Hide file tree
Showing 7 changed files with 359 additions and 730 deletions.
1 change: 0 additions & 1 deletion .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox -->
- [ ] Addresses an existing issue: #0000
- [ ] Ran `yarn null:autoadd`
- [ ] Ran `yarn fastpass`
- [ ] Added/updated relevant unit test(s) (and ran `yarn test`)
- [ ] Verified code coverage for the changes made. Check coverage report at: `<rootDir>/test-results/unit/coverage`
Expand Down
16 changes: 5 additions & 11 deletions tools/strict-null-checks/auto-add.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@
const child_process = require('child_process');
const fs = require('fs');
const path = require('path');
const { collapseCompletedDirectories } = require('./collapse-completed-directories');
const config = require('./config');
const { getUncheckedLeafFiles } = require('./eligible-file-finder');
const { writeTsconfigSync } = require('./write-tsconfig');
const { writeTsConfig } = require('./write-tsconfig');

const repoRoot = config.repoRoot;
const tscPath = path.join(repoRoot, 'node_modules', 'typescript', 'bin', 'tsc');
Expand Down Expand Up @@ -44,35 +43,30 @@ async function main() {

console.log('## Stopping tsc --watch process...');
tscWatchProcess.kill();

console.log('## Collapsing fully null-checked directories into "include" patterns...');
collapseCompletedDirectories(tsconfigPath);
}

async function tryAutoAddStrictNulls(child, tsconfigPath, file) {
const relativeFilePath = path.relative(repoRoot, file).replace(/\\/g, '/');
console.log(`Trying to auto add '${relativeFilePath}'`);

const originalConfig = JSON.parse(fs.readFileSync(tsconfigPath).toString());
originalConfig.files = Array.from(new Set(originalConfig.files.sort()));
originalConfig.exclude = Array.from(new Set(originalConfig.exclude.sort()));

// Config on accept
const newConfig = Object.assign({}, originalConfig);
newConfig.files = Array.from(
new Set(originalConfig.files.concat('./' + relativeFilePath).sort()),
);
newConfig.exclude = originalConfig.exclude.filter(entry => entry !== relativeFilePath);

const buildCompetePromise = waitForBuildComplete(child);

writeTsconfigSync(tsconfigPath, newConfig);
await writeTsConfig(tsconfigPath, newConfig);

const errorCount = await buildCompetePromise;
const success = errorCount === 0;
if (success) {
console.log(`Success`);
} else {
console.log(`Errors (x${errorCount}), skipped`);
writeTsconfigSync(tsconfigPath, originalConfig);
await writeTsConfig(tsconfigPath, originalConfig);
}

return success;
Expand Down
85 changes: 0 additions & 85 deletions tools/strict-null-checks/collapse-completed-directories.js

This file was deleted.

39 changes: 39 additions & 0 deletions tools/strict-null-checks/convert-to-exclude.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

// @ts-check
const fs = require('fs');
const path = require('path');
const config = require('./config');
const { getAllCheckedFiles, getAllEligibleFiles } = require('./eligible-file-finder');
const { writeTsConfig } = require('./write-tsconfig');

const repoRoot = config.repoRoot;
const tsconfigPath = path.join(repoRoot, config.targetTsconfig);

async function main() {
const excludeList = await buildExcludeList();
const tsconfig = JSON.parse(fs.readFileSync(tsconfigPath).toString());

delete tsconfig.files;
tsconfig.include = ['src/**/*.ts', 'src/**/*.tsx'];
tsconfig.exclude = ['**/*.test.ts', '**/*.test.tsx', ...excludeList];

await writeTsConfig(tsconfigPath, tsconfig);
}

async function buildExcludeList() {
const checkedFiles = await getAllCheckedFiles();
const eligibleFiles = await getAllEligibleFiles();

const allUncheckedFiles = eligibleFiles.filter(file => !checkedFiles.has(file));

allUncheckedFiles.sort();

return allUncheckedFiles.map(file => path.relative(repoRoot, file).replace(/\\/g, '/'));
}

main().catch(error => {
console.error(error.stack);
process.exit(1);
});
13 changes: 11 additions & 2 deletions tools/strict-null-checks/eligible-file-finder.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,26 @@ async function getAllCheckedFiles() {
const tsconfigContent = JSON.parse(fs.readFileSync(tsconfigPath).toString());

const set = new Set(
tsconfigContent.files.map(f => path.join(config.repoRoot, f).replace(/\\/g, '/')),
(tsconfigContent.files ?? []).map(f => path.join(config.repoRoot, f).replace(/\\/g, '/')),
);
await Promise.all(
tsconfigContent.include.map(async include => {
(tsconfigContent.include ?? []).map(async include => {
const includePath = path.join(config.repoRoot, include);
const files = await globAsync(includePath);
for (const file of files) {
set.add(file);
}
}),
);
await Promise.all(
(tsconfigContent.exclude ?? []).map(async exclude => {
const excludePath = path.join(config.repoRoot, exclude);
const files = await globAsync(excludePath);
for (const file of files) {
set.delete(file);
}
}),
);
return set;
}

Expand Down
14 changes: 11 additions & 3 deletions tools/strict-null-checks/write-tsconfig.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
const fs = require('fs');
const path = require('path');
const prettier = require('prettier');

module.exports = {
writeTsconfigSync: (tsconfigPath, content) => {
writeTsConfig: async (tsconfigPath, content) => {
let serializedContent = JSON.stringify(content, null, ' ');
serializedContent += '\n';

fs.writeFileSync(tsconfigPath, serializedContent);
let prettierConfigPath = path.join(__dirname, '..', '..', 'prettier.config.js');
let prettierConfig = await prettier.resolveConfig(prettierConfigPath);
let formattedContent = prettier.format(serializedContent, {
...prettierConfig,
filepath: tsconfigPath,
});

fs.writeFileSync(tsconfigPath, formattedContent);
},
};
Loading

0 comments on commit 1ea9642

Please sign in to comment.