From d6eff3a302a00aadff0735bd741bdb8681a1e085 Mon Sep 17 00:00:00 2001 From: Dan Bjorge Date: Tue, 20 Dec 2022 13:34:02 -0500 Subject: [PATCH] chore(null): null checks for web-visualization-config-factory and deps (#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 - [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: `/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 --- .../components/adhoc-static-test-view.tsx | 8 +++++++- .../accessible-names/visualization.tsx | 8 +++++--- src/ad-hoc-visualizations/color/visualization.tsx | 8 +++++--- .../headings/visualization.tsx | 8 +++++--- .../issues/visualization.tsx | 8 +++++--- .../landmarks/visualization.tsx | 8 +++++--- .../needs-review/visualization.tsx | 8 +++++--- .../tab-stops/visualization.tsx | 8 +++++--- src/background/keyboard-shortcut-handler.ts | 4 ++-- .../visualization-configuration-factory.ts | 2 +- src/common/configs/visualization-configuration.ts | 6 +++--- .../web-visualization-configuration-factory.ts | 15 ++++++--------- .../types/displayable-visualization-type-data.ts | 6 ++++++ src/popup/components/diagnostic-view-toggle.tsx | 12 ++++++++++-- .../adhoc-tab-stops-test-view.test.tsx.snap | 4 +++- .../components/adhoc-static-test-view.test.tsx | 4 +++- .../components/adhoc-tab-stops-test-view.test.tsx | 4 +++- .../components/details-list-issues-view.test.tsx | 4 +++- .../components/target-page-changed-view.test.tsx | 4 +++- .../background/keyboard-shortcut-handler.test.ts | 2 +- ...eb-visualization-configuration-factory.test.ts | 6 +----- tsconfig.strictNullChecks.json | 8 +------- 22 files changed, 88 insertions(+), 57 deletions(-) diff --git a/src/DetailsView/components/adhoc-static-test-view.tsx b/src/DetailsView/components/adhoc-static-test-view.tsx index c952e2055e0..aeeeca5fca9 100644 --- a/src/DetailsView/components/adhoc-static-test-view.tsx +++ b/src/DetailsView/components/adhoc-static-test-view.tsx @@ -44,6 +44,12 @@ export const AdhocStaticTestView = NamedFC( !scanData.enabled, ); const displayableData = props.configuration.displayableData; + const adHocDisplayableData = displayableData.adHoc; + if (adHocDisplayableData == null) { + throw new Error( + 'Cannot render AdhocStaticTestView for visualization without adHoc displayableData', + ); + } const stepsText = (): string => { const fastPassProvider = createFastPassProviderWithFeatureFlags( @@ -68,7 +74,7 @@ export const AdhocStaticTestView = NamedFC( visualizationEnabled: scanData.enabled, onToggleClick: clickHandler, title: displayableData.title, - toggleLabel: displayableData.toggleLabel, + toggleLabel: adHocDisplayableData.toggleLabel, content: props.content, guidance: props.guidance, stepsText: stepsText(), diff --git a/src/ad-hoc-visualizations/accessible-names/visualization.tsx b/src/ad-hoc-visualizations/accessible-names/visualization.tsx index a8e98d2c4e1..6b997e3a037 100644 --- a/src/ad-hoc-visualizations/accessible-names/visualization.tsx +++ b/src/ad-hoc-visualizations/accessible-names/visualization.tsx @@ -36,9 +36,11 @@ export const AccessibleNamesAdHocVisualization: VisualizationConfiguration = { shouldShowExportReport: () => false, displayableData: { title: 'Accessible names', - enableMessage: 'Calculating accessible names...', - toggleLabel: 'Show accessible names', - linkToDetailsViewText: 'How to test accessible names', + adHoc: { + enableMessage: 'Calculating accessible names...', + toggleLabel: 'Show accessible names', + linkToDetailsViewText: 'How to test accessible names', + }, }, chromeCommand: '07_toggle-accessibleNames', launchPanelDisplayOrder: 2, diff --git a/src/ad-hoc-visualizations/color/visualization.tsx b/src/ad-hoc-visualizations/color/visualization.tsx index 785a29a9c76..c04ce0d1bf6 100644 --- a/src/ad-hoc-visualizations/color/visualization.tsx +++ b/src/ad-hoc-visualizations/color/visualization.tsx @@ -35,9 +35,11 @@ export const ColorAdHocVisualization: VisualizationConfiguration = { shouldShowExportReport: () => false, displayableData: { title: 'Color', - enableMessage: 'Changing color to grayscale...', - toggleLabel: 'Show grayscale', - linkToDetailsViewText: 'How to test color', + adHoc: { + enableMessage: 'Changing color to grayscale...', + toggleLabel: 'Show grayscale', + linkToDetailsViewText: 'How to test color', + }, }, chromeCommand: '05_toggle-color', launchPanelDisplayOrder: 5, diff --git a/src/ad-hoc-visualizations/headings/visualization.tsx b/src/ad-hoc-visualizations/headings/visualization.tsx index 4d8f0b8e6f0..b83bf98513e 100644 --- a/src/ad-hoc-visualizations/headings/visualization.tsx +++ b/src/ad-hoc-visualizations/headings/visualization.tsx @@ -36,9 +36,11 @@ export const HeadingsAdHocVisualization: VisualizationConfiguration = { shouldShowExportReport: () => false, displayableData: { title: 'Headings', - enableMessage: 'Finding headings...', - toggleLabel: 'Show headings', - linkToDetailsViewText: 'How to test headings', + adHoc: { + enableMessage: 'Finding headings...', + toggleLabel: 'Show headings', + linkToDetailsViewText: 'How to test headings', + }, }, chromeCommand: '03_toggle-headings', launchPanelDisplayOrder: 3, diff --git a/src/ad-hoc-visualizations/issues/visualization.tsx b/src/ad-hoc-visualizations/issues/visualization.tsx index 72d70f30287..8c9fdfb7eac 100644 --- a/src/ad-hoc-visualizations/issues/visualization.tsx +++ b/src/ad-hoc-visualizations/issues/visualization.tsx @@ -49,9 +49,11 @@ export const IssuesAdHocVisualization: VisualizationConfiguration = { . ), - enableMessage: 'Running automated checks...', - toggleLabel: 'Show failures', - linkToDetailsViewText: 'List view and filtering', + adHoc: { + enableMessage: 'Running automated checks...', + toggleLabel: 'Show failures', + linkToDetailsViewText: 'List view and filtering', + }, }, chromeCommand: '01_toggle-issues', launchPanelDisplayOrder: 1, diff --git a/src/ad-hoc-visualizations/landmarks/visualization.tsx b/src/ad-hoc-visualizations/landmarks/visualization.tsx index 48ab1182c3d..3a083e882a7 100644 --- a/src/ad-hoc-visualizations/landmarks/visualization.tsx +++ b/src/ad-hoc-visualizations/landmarks/visualization.tsx @@ -36,9 +36,11 @@ export const LandmarksAdHocVisualization: VisualizationConfiguration = { shouldShowExportReport: () => false, displayableData: { title: 'Landmarks', - enableMessage: 'Finding landmarks...', - toggleLabel: 'Show landmarks', - linkToDetailsViewText: 'How to test landmarks', + adHoc: { + enableMessage: 'Finding landmarks...', + toggleLabel: 'Show landmarks', + linkToDetailsViewText: 'How to test landmarks', + }, }, chromeCommand: '02_toggle-landmarks', launchPanelDisplayOrder: 2, diff --git a/src/ad-hoc-visualizations/needs-review/visualization.tsx b/src/ad-hoc-visualizations/needs-review/visualization.tsx index 874b76cbe6b..11ca64aaa2f 100644 --- a/src/ad-hoc-visualizations/needs-review/visualization.tsx +++ b/src/ad-hoc-visualizations/needs-review/visualization.tsx @@ -48,9 +48,11 @@ export const NeedsReviewAdHocVisualization: VisualizationConfiguration = { . ), - enableMessage: 'Running needs review checks...', - toggleLabel: 'Show elements needing review', - linkToDetailsViewText: 'List view and filtering', + adHoc: { + enableMessage: 'Running needs review checks...', + toggleLabel: 'Show elements needing review', + linkToDetailsViewText: 'List view and filtering', + }, }, chromeCommand: '06_toggle-needsReview', launchPanelDisplayOrder: 6, diff --git a/src/ad-hoc-visualizations/tab-stops/visualization.tsx b/src/ad-hoc-visualizations/tab-stops/visualization.tsx index d176b449327..27af4dac6d0 100644 --- a/src/ad-hoc-visualizations/tab-stops/visualization.tsx +++ b/src/ad-hoc-visualizations/tab-stops/visualization.tsx @@ -37,9 +37,11 @@ export const TabStopsAdHocVisualization: VisualizationConfiguration = { shouldShowExportReport: () => true, displayableData: { title: 'Tab stops', - enableMessage: 'Start pressing Tab to start visualizing tab stops.', - toggleLabel: 'Show tab stops', - linkToDetailsViewText: 'How to test tab stops', + adHoc: { + enableMessage: 'Start pressing Tab to start visualizing tab stops.', + toggleLabel: 'Show tab stops', + linkToDetailsViewText: 'How to test tab stops', + }, }, chromeCommand: '04_toggle-tabStops', launchPanelDisplayOrder: 4, diff --git a/src/background/keyboard-shortcut-handler.ts b/src/background/keyboard-shortcut-handler.ts index a396d22c447..86799f4e4b6 100644 --- a/src/background/keyboard-shortcut-handler.ts +++ b/src/background/keyboard-shortcut-handler.ts @@ -135,9 +135,9 @@ export class KeyboardShortcutHandler { if (!scanData.enabled) { const displayableVisualizationData = configuration.displayableData; - if (displayableVisualizationData) { + if (displayableVisualizationData?.adHoc?.enableMessage) { this.notificationCreator.createNotification( - displayableVisualizationData.enableMessage, + displayableVisualizationData.adHoc.enableMessage, ); } } diff --git a/src/common/configs/visualization-configuration-factory.ts b/src/common/configs/visualization-configuration-factory.ts index 47333c3e596..f1350e4a931 100644 --- a/src/common/configs/visualization-configuration-factory.ts +++ b/src/common/configs/visualization-configuration-factory.ts @@ -10,7 +10,7 @@ import { VisualizationConfiguration } from './visualization-configuration'; // The interface is split to avoid circular dependencies (this interface is a commonly used react // prop/dep for components which WebVisualizationConfigurationFactory indirectly points to) export interface VisualizationConfigurationFactory { - getConfigurationByKey(key: string): VisualizationConfiguration; + getConfigurationByKey(key: string): VisualizationConfiguration | undefined; getConfiguration(visualizationType: VisualizationType): VisualizationConfiguration; getChromeCommandToVisualizationTypeMap(): DictionaryStringTo; forEachConfig(callback: ForEachConfigCallback): void; diff --git a/src/common/configs/visualization-configuration.ts b/src/common/configs/visualization-configuration.ts index b2b2b522b60..a21df42dd94 100644 --- a/src/common/configs/visualization-configuration.ts +++ b/src/common/configs/visualization-configuration.ts @@ -21,9 +21,9 @@ export interface VisualizationConfiguration extends AssessmentVisualizationConfi instanceMap?: DictionaryStringTo, ) => void; displayableData: DisplayableVisualizationTypeData; - chromeCommand: string; - launchPanelDisplayOrder: number; - adhocToolsPanelDisplayOrder: number; + chromeCommand: string | null; + launchPanelDisplayOrder: number | null; + adhocToolsPanelDisplayOrder: number | null; analyzerProgressMessageType?: string; analyzerTerminatedMessageType?: string; guidance?: ContentPageComponent; diff --git a/src/common/configs/web-visualization-configuration-factory.ts b/src/common/configs/web-visualization-configuration-factory.ts index d8256f89303..20bf64cb5f9 100644 --- a/src/common/configs/web-visualization-configuration-factory.ts +++ b/src/common/configs/web-visualization-configuration-factory.ts @@ -43,13 +43,13 @@ export class WebVisualizationConfigurationFactory implements VisualizationConfig }; } - public getConfigurationByKey(key: string): VisualizationConfiguration { + public getConfigurationByKey(key: string): VisualizationConfiguration | undefined { return find(values(this.configurationByType), config => config.key === key); } public getConfiguration(visualizationType: VisualizationType): VisualizationConfiguration { if (this.mediumPassProvider?.isValidType(visualizationType)) { - const assessment = this.mediumPassProvider.forType(visualizationType); + const assessment = this.mediumPassProvider.forType(visualizationType)!; return this.buildAssessmentConfiguration( assessment, TestMode.MediumPass, @@ -58,7 +58,7 @@ export class WebVisualizationConfigurationFactory implements VisualizationConfig } if (this.fullAssessmentProvider.isValidType(visualizationType)) { - const assessment = this.fullAssessmentProvider.forType(visualizationType); + const assessment = this.fullAssessmentProvider.forType(visualizationType)!; return this.buildAssessmentConfiguration( assessment, TestMode.Assessments, @@ -114,7 +114,7 @@ export class WebVisualizationConfigurationFactory implements VisualizationConfig const getIdentifier = (requirementKey: string) => { const requirement = assessment.requirements.find(req => req.key === requirementKey); - return `${testMode}-${requirement.key}`; + return `${testMode}-${requirement!.key}`; }; const getStoreData = (data: TestsEnabledState) => { @@ -128,12 +128,9 @@ export class WebVisualizationConfigurationFactory implements VisualizationConfig adhocToolsPanelDisplayOrder: null, displayableData: { title: assessment.title, - noResultsFound: null, - enableMessage: null, - toggleLabel: null, - linkToDetailsViewText: null, + adHoc: null, }, - shouldShowExportReport: null, + shouldShowExportReport: () => false, getIdentifier, getStoreData, messageConfiguration, diff --git a/src/common/types/displayable-visualization-type-data.ts b/src/common/types/displayable-visualization-type-data.ts index d00ba6524b7..21388aec4cc 100644 --- a/src/common/types/displayable-visualization-type-data.ts +++ b/src/common/types/displayable-visualization-type-data.ts @@ -3,6 +3,12 @@ export interface DisplayableVisualizationTypeData { title: string; subtitle?: JSX.Element; + + // populated if-and-only-if visualization is for an ad-hoc tool + adHoc: AdHocDisplayableVisualizationTypeData | null; +} + +export interface AdHocDisplayableVisualizationTypeData { enableMessage: string; toggleLabel: string; linkToDetailsViewText: string; diff --git a/src/popup/components/diagnostic-view-toggle.tsx b/src/popup/components/diagnostic-view-toggle.tsx index 355a9a04bb5..21d1efc2b0a 100644 --- a/src/popup/components/diagnostic-view-toggle.tsx +++ b/src/popup/components/diagnostic-view-toggle.tsx @@ -68,13 +68,21 @@ export class DiagnosticViewToggle extends React.Component<
{displayableData.title}
{this.renderToggleOrSpinner()}
-
{this.renderLink(displayableData.linkToDetailsViewText)}
+ {this.renderLinkToDetailsView()} {this.renderShortcut()}
); } - private renderShortcut(): JSX.Element { + private renderLinkToDetailsView(): JSX.Element | null { + const linkText = this.configuration.displayableData.adHoc?.linkToDetailsViewText; + if (linkText == null) { + return null; + } + return
{this.renderLink(linkText)}
; + } + + private renderShortcut(): JSX.Element | null { if (!this.configuration.chromeCommand) { return null; } diff --git a/src/tests/unit/tests/DetailsView/components/__snapshots__/adhoc-tab-stops-test-view.test.tsx.snap b/src/tests/unit/tests/DetailsView/components/__snapshots__/adhoc-tab-stops-test-view.test.tsx.snap index 38baf4b3ab3..c5fa139da20 100644 --- a/src/tests/unit/tests/DetailsView/components/__snapshots__/adhoc-tab-stops-test-view.test.tsx.snap +++ b/src/tests/unit/tests/DetailsView/components/__snapshots__/adhoc-tab-stops-test-view.test.tsx.snap @@ -278,8 +278,10 @@ exports[`AdhocTabStopsTestView render should return target page changed view as detailsViewActionMessageCreator={[Function]} displayableData={ { + "adHoc": { + "toggleLabel": "test toggle label", + }, "title": "test title", - "toggleLabel": "test toggle label", } } toggleClickHandler={[Function]} diff --git a/src/tests/unit/tests/DetailsView/components/adhoc-static-test-view.test.tsx b/src/tests/unit/tests/DetailsView/components/adhoc-static-test-view.test.tsx index 046d10f49f6..efb2d8d99d5 100644 --- a/src/tests/unit/tests/DetailsView/components/adhoc-static-test-view.test.tsx +++ b/src/tests/unit/tests/DetailsView/components/adhoc-static-test-view.test.tsx @@ -38,7 +38,9 @@ describe('AdhocStaticTestView', () => { ); displayableDataStub = { title: 'test title', - toggleLabel: 'test toggle label', + adHoc: { + toggleLabel: 'test toggle label', + }, } as DisplayableVisualizationTypeData; scanDataStub = { enabled: true, diff --git a/src/tests/unit/tests/DetailsView/components/adhoc-tab-stops-test-view.test.tsx b/src/tests/unit/tests/DetailsView/components/adhoc-tab-stops-test-view.test.tsx index c43d1825d66..ebb9701365a 100644 --- a/src/tests/unit/tests/DetailsView/components/adhoc-tab-stops-test-view.test.tsx +++ b/src/tests/unit/tests/DetailsView/components/adhoc-tab-stops-test-view.test.tsx @@ -46,7 +46,9 @@ describe('AdhocTabStopsTestView', () => { ); displayableDataStub = { title: 'test title', - toggleLabel: 'test toggle label', + adHoc: { + toggleLabel: 'test toggle label', + }, } as DisplayableVisualizationTypeData; scanDataStub = { enabled: true, diff --git a/src/tests/unit/tests/DetailsView/components/details-list-issues-view.test.tsx b/src/tests/unit/tests/DetailsView/components/details-list-issues-view.test.tsx index ac0a6876e8b..01456f2c8a5 100644 --- a/src/tests/unit/tests/DetailsView/components/details-list-issues-view.test.tsx +++ b/src/tests/unit/tests/DetailsView/components/details-list-issues-view.test.tsx @@ -33,7 +33,9 @@ describe('DetailsListIssuesView', () => { displayableDataStub = { title: 'test title', subtitle: <>test subtitle, - toggleLabel: 'test toggle label', + adHoc: { + toggleLabel: 'test toggle label', + }, } as DisplayableVisualizationTypeData; scanDataStub = { enabled: true, diff --git a/src/tests/unit/tests/DetailsView/components/target-page-changed-view.test.tsx b/src/tests/unit/tests/DetailsView/components/target-page-changed-view.test.tsx index 6c462bd1881..1664c55a78f 100644 --- a/src/tests/unit/tests/DetailsView/components/target-page-changed-view.test.tsx +++ b/src/tests/unit/tests/DetailsView/components/target-page-changed-view.test.tsx @@ -23,7 +23,9 @@ describe('TargetPageChangedView', () => { const clickHandlerStub: () => void = () => {}; const displayableData = { title: 'test title', - toggleLabel: 'test toggle label', + adHoc: { + toggleLabel: 'test toggle label', + }, subtitle, } as DisplayableVisualizationTypeData; const detailsViewActionMessageCreator = {} as DetailsViewActionMessageCreator; diff --git a/src/tests/unit/tests/background/keyboard-shortcut-handler.test.ts b/src/tests/unit/tests/background/keyboard-shortcut-handler.test.ts index 661670595d4..0c603d42f5b 100644 --- a/src/tests/unit/tests/background/keyboard-shortcut-handler.test.ts +++ b/src/tests/unit/tests/background/keyboard-shortcut-handler.test.ts @@ -244,7 +244,7 @@ describe('KeyboardShortcutHandler', () => { const configuration = visualizationConfigurationFactory.getConfiguration(visualizationType); - const enableMessage = configuration.displayableData.enableMessage; + const enableMessage = configuration.displayableData.adHoc.enableMessage; notificationCreatorMock .setup(nc => nc.createNotification(enableMessage)) .verifiable(Times.once()); diff --git a/src/tests/unit/tests/common/configs/web-visualization-configuration-factory.test.ts b/src/tests/unit/tests/common/configs/web-visualization-configuration-factory.test.ts index ad3fed55cf0..25aa49fda72 100644 --- a/src/tests/unit/tests/common/configs/web-visualization-configuration-factory.test.ts +++ b/src/tests/unit/tests/common/configs/web-visualization-configuration-factory.test.ts @@ -356,12 +356,8 @@ describe('WebVisualizationConfigurationFactory', () => { adhocToolsPanelDisplayOrder: null, displayableData: { title: expectedDisplayableTitle, - noResultsFound: null, - enableMessage: null, - toggleLabel: null, - linkToDetailsViewText: null, + adHoc: null, }, - shouldShowExportReport: null, messageConfiguration: expectedMessageConfig, }; } diff --git a/tsconfig.strictNullChecks.json b/tsconfig.strictNullChecks.json index fba9d2981c4..8f3940cf63b 100644 --- a/tsconfig.strictNullChecks.json +++ b/tsconfig.strictNullChecks.json @@ -239,13 +239,6 @@ "./src/common/components/toast.tsx", "./src/common/components/visualization-toggle.tsx", "./src/common/components/with-store-subscription.tsx", - "./src/common/configs/a11y-insights-rule-resources.ts", - "./src/common/configs/assessment-visualization-configuration.ts", - "./src/common/configs/inspect-configuration-factory.ts", - "./src/common/configs/test-mode.ts", - "./src/common/configs/unified-result-property-configurations.tsx", - "./src/common/configs/visualization-configuration-factory.ts", - "./src/common/configs/visualization-configuration.ts", "./src/common/date-provider.ts", "./src/common/document-manipulator.ts", "./src/common/dropdown-click-handler.ts", @@ -578,6 +571,7 @@ "src/common/action/**/*", "src/common/browser-adapters/**/*", "src/common/components/**/*", + "src/common/configs/**/*", "src/common/configuration/**/*", "src/common/constants/**/*", "src/common/flux/**/*",