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

Codebase should use strict null checks #2869

Open
dbjorge opened this issue Jun 12, 2020 · 0 comments
Open

Codebase should use strict null checks #2869

dbjorge opened this issue Jun 12, 2020 · 0 comments

Comments

@dbjorge
Copy link
Contributor

dbjorge commented Jun 12, 2020

How to help

Migrating to strict null checks is in progress, and we have some scripts (heavily based on guidance and scripts from the VS Code team) in place to help. (Note: all of these assume you have already run yarn install and yarn build once first):

$ # Verify that there are no null-safety issues in the tsconfig.strictNullChecks.json files
$ yarn null:check

$ # Start a "watch" window to continuously verify as you make changes/fixes
$ yarn null:check --watch

$ # Print a markdown-formatted checklist of files that are good candidates to try to make null-safe (ie, files with no transitive dependencies on any other non-null-safe files)
$ yarn null:find

$ # Automatically add any files null:find would output to tsconfig.strictNullChecks.json, if-and-only-if they can be added without introducing new null safety errors
$ yarn null:autoadd

The recommended workflow for contributing a null safety improvment is:

  1. On the side, start a yarn null:check --watch process
  2. Run yarn null:find to see a list of candidate files to work on
  3. Add a candidate file to the files list in tsconfig.strictNullChecks.json
  4. Fix any issues that pop up in the watch process
  5. Run yarn null:autoadd to pick up any additional files that might have been incidentally fixed as a result of your changes
  6. Optionally, repeat steps 2-5 until you have a reasonably scoped PR (try not to make them too big!)
  7. Send a PR referencing this issue
  8. Update the widget on our team standup dashboard with progress!

Background

Per 6/12/20, there was broad (but not unanimous) agreement among the engineering team that we would like to enable typescript's strictNullChecks: true config in our codebase; most participants felt that the errors that could be prevented with this particular check outweighed the engineering cost of retrofitting the code to allow it.

To enable this, we'll be following the same strategy that VS Code did; they outline the migration strategy they used in great detail in their blog post Strict null checking Visual Studio Code. To summarize it as it applies to our codebase:

  1. PR chore: start incremental migration to strictNullChecks #2835 adds a new tsconfig.strictNullChecks.json that performs null checking on the set of already-strictNullChecks-clean files
  2. PR chore: start incremental migration to strictNullChecks #2835 sets up stay-clean verification in yarn fastpass and PR/CI builds
  3. PR chore(strict-null): add strict null check migration tools to repo #2957 adds the migration tools to the repo
  4. We've added a widget to our team standup dashboard to track progress (for now, it needs to be manually updated as progress is made)

Potential action items upon completion:

  1. Migrate existing schema generation in LoadAssessmentDataValidator to use JSON Type Definition which should lessen the maintenance burden for the schema in the future.
@dbjorge dbjorge self-assigned this Jun 12, 2020
dbjorge added a commit that referenced this issue Jun 16, 2020
#### Description of changes

This PR, plus the scripts in [dbjorge/aiweb-strict-null-check-migration-tools](https://github.com/dbjorge/aiweb-strict-null-check-migration-tools), creates the initial infrastructure for an incremental migration to `strictNullChecks: true`, following the [same strategy that VS Code used](https://code.visualstudio.com/blogs/2019/05/23/strict-null).

* Adds a secondary `tsconfig` file at `tsconfig.strictNullChecks.json` which is the same as our main/normal tsconfig file except that it enables `strictNullChecks` and only runs on the already-compliant parts of our code
* Populates the initial `files` and `includes` of the `strictNullChecks` config with the subset of `/src/` that is already compliant (by using `autoAdd` from [aiweb-strict-null-check-migration-tools](https://github.com/dbjorge/aiweb-strict-null-check-migration-tools)
* Adds a new `yarn null:check` command to compile against the new strictNullChecks config (note, an especially useful option during cleanup work is `yarn null:check --watch`)
* Adds `yarn null:check` into `yarn fastpass`
* Adds `yarn null:check` into PR/CI build

The ongoing effort to incrementally bring the codebase to null safety would amount to iteratively using [aiweb-strict-null-check-migration-tools](https://github.com/dbjorge/aiweb-strict-null-check-migration-tools) to find the next candidate files, fixing up null safety issues there, and using that repo's `autoAdd` tool to bring in any transitively-fixed cases.

#### 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 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
dbjorge added a commit that referenced this issue Jun 29, 2020
)

#### Description of changes

This integrates the strict null checking tools that I migrated from VS Code's as an experiment in https://github.com/dbjorge/aiweb-strict-null-check-migration-tools into the `/tools/` folder of our repo, to make it easier for others to use them.

As part of the migration, I eliminated the separate `package.json` machinery for the tools and had them just re-use the same `node_modules` as the main project (in particular, re-using the correct typescript version). I also added entry point run-scripts for the tools to our root-level package.json, so the source for the tools should be an implementation detail for most purposes of actually running null check migration work; usage is just `yarn null:find` and `yarn null:autoadd` from the root, to match the previously-added `yarn null:check`.

I've updated #2869 with instructions for how to use these scripts to contribute to the strict null check migration work.

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox -->
- [x] Addresses an existing issue: tools to help address #2869 
- [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
dbjorge added a commit that referenced this issue Jun 30, 2020
…2990)

#### Description of changes

This PR adds all of the files matched by `yarn null:find` of form `/src/common/**/*.ts` to the strict null checks list.

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox -->
- [x] Addresses an existing issue: part of #2869
- [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
dbjorge added a commit that referenced this issue Jul 7, 2020
#### Description of changes

This PR just runs `yarn null:autoadd`, mostly covering new files added with the android-setup feature that are already compliant.

I also made a minor update to the autoadd script to make it write its output in a prettier-compatible format, to avoid having to manually run fastpass:fix afterwards.

#### 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 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
dbjorge added a commit that referenced this issue Jul 7, 2020
#### Description of changes

This implements a new script that generates a progress message for use with our standup dashboard. `yarn null:progress` emits stdout like this:

```markdown
## Web strict-null progress

**29%** complete (**387**/1328 non-test files)

*Contribute at [#2869](#2869). Last update: Mon Jul 06 2020*
```

...which renders in the dashboard widget like this:

![screenshot of above output as-rendered in dashboard](https://user-images.githubusercontent.com/376284/86670541-9a75a180-bfa9-11ea-99f2-730038d68a71.png)

#### 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 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
dbjorge added a commit that referenced this issue Jul 7, 2020
#### Description of changes

Adds test utility files noted by `yarn null:find` to strict-null set.

#### 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 fastpass`
- [x] 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
dbjorge added a commit that referenced this issue Jul 7, 2020
#### Description of changes

null-correctness for axe-utils plus the handful of implicitly references `*.d.ts` files

#### 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 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
dbjorge added a commit that referenced this issue Sep 10, 2020
…directory (#3329)

#### Description of changes

This PR improves the readability of `yarn null:autoadd` output by making it automatically collapse cases where an entire directory is detected as "completed" into a directory-wise include pattern, rather than listing out individual files.

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox -->
- [x] Addresses an existing issue: part of #2869
- [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
dbjorge added a commit that referenced this issue Sep 11, 2020
#### Description of changes

The primary impact/motivation here was to fix an issue where `yarn null:find` wasn't outputting all the files it was supposed to (as it turns out, this was windows-specific). The fix for this is on line 39 of `import-finder.js` (different parts of the tool weren't in agreement about normalizing slashes, which was breaking some of the filtering logic).

As part of fixing/debugging this, I did a fair amount of refactoring/cleanup to null:find. I didn't separate out the cleanup into a separate PR like I normally would because this is a short-lived dev-only tool that doesn't require as thorough review as most of the repo's code.

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox -->
- [x] Addresses an existing issue: related to #2869
- [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
dbjorge added a commit that referenced this issue Sep 11, 2020
#### Description of changes

Previously, if `yarn null:autoadd` added a file to the strictNullChecks config which enabled a previously-ineligible file to be considered for auto-add, you'd have to run `yarn null:autoadd` again and eat the cost of it spinning up a new tsc watch process and rescanning all the already-known-failures.

With this PR, the tool will do this for you automatically, re-using both the watch process and previously-established knowledge of known failures.

`yarn null:progress` update with this PR:

## Web strict-null progress

**42%** complete (**558**/1334 non-test files)

*Contribute at [#2869](#2869). Last update: Fri Sep 11 2020*

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox -->
- [x] Addresses an existing issue: part of #2869
- [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
dbjorge added a commit that referenced this issue Sep 15, 2020
…manual attention (#3341)

#### Description of changes

Cleanup for a few new strict-null-check cases

#### 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 fastpass`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] 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
dbjorge added a commit that referenced this issue Sep 15, 2020
… common/stores (#3347)

#### Description of changes

Assortment of minor null strictness fixes for the most-referenced candidate files from `yarn null:find`

#### 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 fastpass`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] 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
dbjorge added a commit that referenced this issue Sep 15, 2020
#3359)

#### Description of changes

Another round of the top ten `yarn null:find` results, plus the extra dependent files that `yarn null:autoadd` revealed were fixed as a side effect of the first fixes.

The most interesting/notable change here is in the error handling improvements to `target-tab-finder.ts`; previously, a few edge cases the browser could return to us would result in a string error or a "cannot reference property url of undefined" error, where now they will throw more meaningful `Error` objects. Tests were updated to accomodate.

Before:
```sh
> yarn null:progress

**43%** complete (**573**/1343 non-test files)
```

After:
```sh
> yarn null:progress

**45%** complete (**599**/1343 non-test files)
```

#### 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 fastpass`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] 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
madalynrose added a commit that referenced this issue Sep 1, 2022
#### Details

This PR fixes strict null checks for a few different files. These files were selected by running `yarn null:find`, added to `tsconfig.strictNullChecks.json` one-by-one, judged on whether their resolutions were straightforward enough to not require their own PR (removed if complex), and addressed.

I recommend going through this PR commit-by-commit, as each file from `yarn null:find` is scoped to a single commit.
* `src/common/rule-based-view-model-provider.ts`: allow various methods to return `null`
* `src/background/stores/tab-store.ts`: update `TabStoreData` to accept undefined values for `url`, `title`, and `id`, as that is what is returned from the `chrome.tabs` API
* `accept undefined values for `description`, `url`, and `guidance` for RuleResults, as returned by axe
* `src/DetailsView/components/interactive-header.tsx`: update InteractiveHeader to exclude `items` (an optional param) instead of passing `null`
* make `originalResult` optional and default to undefined, as it is optional in the data returned from axe
 
##### Motivation

#2869

##### Context

More PRs like this to follow, trying to keep PRs relatively small and well-chunked.

#### 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`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] 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

Co-authored-by: madalynrose <[email protected]>
dbjorge added a commit that referenced this issue Dec 1, 2022
#### Details

This PR adds a bunch of `injected/visualization` files to the
strictNullCheck set. To do this, I introduced an interface seam for
`DialogRenderer` (since it has dependencies on lots of non-null-clean
components) and then did two rounds of `yarn null:find`/`yarn
null:autoadd`.

I also included a few fixes for dialog-renderer-impl that I started
making before deciding to introduce the interface seam. It is
intentionally omitted from the strictNullChecks config since it still
has non-clean dependencies.

##### Motivation

See #2869

##### Context

n/a

#### 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`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] 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
dbjorge added a commit that referenced this issue Dec 2, 2022
#### Details

Adds the UI package's impl directory + its dependants to the strict null
check set.

##### Motivation

#2869

##### Context

n/a

#### 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`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] 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
dbjorge added a commit that referenced this issue Dec 5, 2022
…#6234)

#### Details

This PR continues from #6223 and fixes another few layers of
`injected/visualization` files to be strict null check compatible. It
substantially refactors `accessibile-name-formatter` and somewhat
refactors `landmark-formatter` - I manually smoke tested those ad hoc
tools to verify that they still worked after the changes

##### Motivation

#2869

##### Context

n/a

#### 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`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] 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
dbjorge added a commit that referenced this issue Dec 6, 2022
…, deps (#6244)

#### Details

This PR continues the work from #6226 and #6223, adding another layer of
injected files to the strict null check list.

The only case with a notable behavioral change is that axe-core
technically documents in its typings that it's allowed to omit a "how to
fix" section from results, but if it were to actually do so, our cards
UI would currently crash. I don't think axe-core actually omits that
info in practice (I suspect it's there to accomodate custom rules), but
now if it were to do so, we'd omit the section from the card rather than
crashing (similar to how we handle results with missing snippets).

This ended up enabling a *lot* of files to be autoadded! It brings us
from ~72% null-strict to ~78%

```markdown
## Web strict-null progress

**78%** complete (**1210**/1561 non-test files)

*Contribute at [#2869](#2869). Last update: Mon Dec 05 2022*
Done in 1.00s.
```

##### Motivation

#2869

##### Context

n/a

#### 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`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] 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
dbjorge added a commit that referenced this issue Dec 17, 2022
#### Details

This PR adds the injected DrawingController + a few related/dependent
files to the strict null check list.

The most interesting of the changes was updating the `DrawerInitData`
type to not be generic. Previously, it was genericized over the data
type an individual drawer expected (in particular, svg-drawer expects to
only be used with something special). But the generic type usage meant
that in practice, the individual drawers were written looking like they
could safely assume what type of input they were given with the relevant
`any` conversions happening in the drawer controller/initiator, which
wasn't really accurate - if a type assumption was wrong, it'd be the
individual drawer that would have to be aware of the mistake. This
change moves the type assertions in question to the drawer.

Overall, these changes enabled autoadding several more assessment
folders and brought us from 78% to 80% null checked:

```
❯ yarn null:progress
yarn run v1.22.19
$ node ./tools/strict-null-checks/progress.js
## Web strict-null progress

**80%** complete (**1244**/1562 non-test files)

*Contribute at [#2869](#2869). Last update: Fri Dec 16 2022*
```

##### Motivation

#2869

##### Context

n/a

#### 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`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] 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
dbjorge added a commit that referenced this issue Dec 19, 2022
#### Details

This PR adds `assessment-builder`, `assessment-provider`, and most
individual assessment `test-steps` to the strict null check config.

The only particularly interesting conversion was in
`assessments-requirements-filter` - I ended up converting a pattern that
used a mutating `filter` callback followed by a `map` to use a single
`reduce` call to implement a `filterMap` pattern, so that typescript
could see the relevant type information within the same scope instead of
being split into several scopes.

```md
## Web strict-null progress

**81%** complete (**1272**/1562 non-test files)

*Contribute at [#2869](#2869). Last update: Fri Dec 16 2022*
```

##### Motivation

#2869

##### Context

n/a

#### 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`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] 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
dbjorge added a commit that referenced this issue Dec 20, 2022
#6279)

#### Details

This PR adds just a single file to the strict null check list
(`src/common/configs/web-visualization-configuration-factory.ts`), but
I've given it its own PR since one of the changes touched a lot of
files. Specifically, the factory involved initializing as null several
visualization fields that were present in the
`VisualizationConfiguration` type but never used except by ad-hoc
checks.

In one case, there was a pattern that I thought was a bit misleading
where a bag of UI strings called `displayableData` contained a mix of
properties which all visualizations were expected to provide (`title`)
vs some which only ad-hoc visualizations were expected to provide
(several properties used to store the labels/etc on the ad hoc popup
panel). I separated these so the ad-hoc specific ones were under a new
`adHoc` grouping and included a comment on the grouping that clarified
when this could be expected to be null or not. This involved touching
all the files which use these properties.

This isn't quite as good as using separate typings for the ad-hoc vs
non-ad-hoc options to make the type system enforce that comment, but I
felt it was a reasonable middle ground for how much existing code to
re-write in order to make this change.

##### Motivation

#2869

##### Context

n/a

#### 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`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] 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
dbjorge added a commit that referenced this issue Dec 20, 2022
…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
@dbjorge dbjorge removed their assignment May 22, 2023
@DaveTryon DaveTryon moved this from Needs Triage to Old backlog in Accessibility Insights Jun 28, 2023
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

1 participant