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): null strictness for injected/components, /visualizations, deps #6244

Merged
merged 9 commits into from
Dec 6, 2022

Conversation

dbjorge
Copy link
Contributor

@dbjorge dbjorge commented Dec 5, 2022

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%

## Web strict-null progress

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

*Contribute at [#2869](https://github.com/microsoft/accessibility-insights-web/issues/2869). Last update: Mon Dec 05 2022*
Done in 1.00s.
Motivation

#2869

Context

n/a

Pull request checklist

  • Addresses an existing issue: Codebase should use strict null checks #2869
  • 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
  • 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 marked this pull request as ready for review December 5, 2022 22:06
@dbjorge dbjorge requested a review from a team as a code owner December 5, 2022 22:06
@dbjorge dbjorge changed the title chore(null): null strictness for another layer of injected files chore(null): null strictness for injected/analyzer/** and siblings Dec 5, 2022
@dbjorge dbjorge changed the title chore(null): null strictness for injected/analyzer/** and siblings chore(null): null strictness for injected/components/** and siblings Dec 5, 2022
@dbjorge dbjorge changed the title chore(null): null strictness for injected/components/** and siblings chore(null): null strictness for injected/components, /visualizations, deps Dec 6, 2022
)
.join(';');
}
public static getSelectorFromTarget = (target: Target): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this still return undefined if target is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, intentionally; there are still a bunch of indirect usages in non-strict-null files right now (mostly via assessment-builder) and I haven't analyzed all of them yet, so I wanted to simultaneously:

  • keep the original behavior intact to avoid breaking those cases accidentally (that's why there's still unit tests for this behavior as well)
  • strengthen the typing so that cases that are strict-null-checked and can verify that they don't pass undefined targets will also understand that they won't get back undefined selectors given that they're using it right.

I'll add a comment to that effect, I agree it isn't obvious enough from just reading the code that this would be intentional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(and I'll do the same for getTargetFromSelector, for the same reasons)

@dbjorge dbjorge merged commit 0301cfe into microsoft:main Dec 6, 2022
@dbjorge dbjorge deleted the null-20221205 branch December 6, 2022 19:09
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