Skip to content

Commit

Permalink
chore(null): null strictness fixes for devtools, e2e page controller,…
Browse files Browse the repository at this point in the history
… 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
  • Loading branch information
dbjorge authored Sep 15, 2020
1 parent a1f0455 commit c7a56b1
Show file tree
Hide file tree
Showing 10 changed files with 31 additions and 19 deletions.
4 changes: 2 additions & 2 deletions src/assessments/assessment-default-message-generator.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ export type IMessageGenerator = (
export type IGetMessageGenerator = (
generator: AssessmentDefaultMessageGenerator,
) => IMessageGenerator;
export interface DefaultMessageInterface {
export type DefaultMessageInterface = {
message: JSX.Element;
instanceCount: number;
}
} | null;

function failingInstances(result: TestStepResult): boolean {
return result.status !== ManualTestStatus.PASS;
Expand Down
2 changes: 1 addition & 1 deletion src/common/components/flagged-component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export interface FlaggedComponentProps {
}

export class FlaggedComponent extends React.Component<FlaggedComponentProps> {
public render(): JSX.Element {
public render(): JSX.Element | null {
const flagName = this.props.featureFlag;

if (this.props.featureFlagStoreData != null && this.props.featureFlagStoreData[flagName]) {
Expand Down
2 changes: 1 addition & 1 deletion src/common/components/with-store-subscription.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export function withStoreSubscription<P extends WithStoreSubscriptionProps<S>, S
constructor(props: P) {
super(props);
if (this.hasStores()) {
this.state = this.props.deps.storesHub.getAllStoreData();
this.state = this.props.deps.storesHub.getAllStoreData()!;
} else {
this.state = {} as S;
}
Expand Down
4 changes: 2 additions & 2 deletions src/common/store-proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ import { StoreUpdateMessage } from './types/store-update-message';
export class StoreProxy<TState> extends Store implements BaseStore<TState> {
private state: TState;
private storeId: string;
private tabId: number;
private tabId?: number;
private browserAdapter: BrowserAdapter;

constructor(storeId: string, browserAdapter: BrowserAdapter, tabId: number = null) {
constructor(storeId: string, browserAdapter: BrowserAdapter, tabId?: number) {
super();
this.storeId = storeId;
this.browserAdapter = browserAdapter;
Expand Down
2 changes: 1 addition & 1 deletion src/common/stores/base-client-stores-hub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export class BaseClientStoresHub<T> implements ClientStoresHub<T> {
});
}

public getAllStoreData(): T {
public getAllStoreData(): T | null {
if (!this.hasStores()) {
return null;
}
Expand Down
2 changes: 1 addition & 1 deletion src/common/stores/client-stores-hub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@ export interface ClientStoresHub<T> {
removeChangedListenerFromAllStores(listener: () => void): void;
hasStores(): boolean;
hasStoreData(): boolean;
getAllStoreData(): T;
getAllStoreData(): T | null;
}
2 changes: 1 addition & 1 deletion src/tests/end-to-end/common/browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ export class Browser {
resolve(tabs[0].id),
);
});
});
}, null);
}

private async waitForBackgroundPageMatching(
Expand Down
18 changes: 13 additions & 5 deletions src/tests/end-to-end/common/page-controllers/page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,12 @@ export class Page {
forceEventFailure(`'pageerror' (console.error): ${serializeError(error)}`);
});
underlyingPage.on('requestfailed', request => {
const failure = request.failure()!;
const url = request.url();
// Checking for 'fonts' and 'icons' in url as a workaround for #923
if (!includes(url, 'fonts') && !includes(url, 'icons')) {
forceEventFailure(
`'requestfailed' from '${url}' with errorText: ${request.failure().errorText}`,
`'requestfailed' from '${url}' with errorText: ${failure.errorText}`,
);
}
});
Expand Down Expand Up @@ -100,7 +101,7 @@ export class Page {
await this.screenshotOnError(async () => await this.underlyingPage.close());
}

public async evaluate<R, Arg>(fn: PageFunction<Arg, R>, arg?: Arg): Promise<R> {
public async evaluate<R, Arg>(fn: PageFunction<Arg, R>, arg: Arg): Promise<R> {
const timeout = DEFAULT_PAGE_ELEMENT_WAIT_TIMEOUT_MS;
// We don't wrap this in screenshotOnError because Playwright serializes evaluate() and
// screenshot() such that screenshot() will always time out if evaluate is still running.
Expand All @@ -111,7 +112,7 @@ export class Page {
public async waitForSelector(
selector: string,
options?: WaitForSelectorOptions,
): Promise<Playwright.ElementHandle<Element>> {
): Promise<Playwright.ElementHandle<Element> | null> {
return await this.screenshotOnError(
async () =>
await this.underlyingPage.waitForSelector(selector, {
Expand All @@ -134,7 +135,9 @@ export class Page {
});
}

public async getSelectorElement(selector: string): Promise<Playwright.ElementHandle<Element>> {
public async getSelectorElement(
selector: string,
): Promise<Playwright.ElementHandle<Element> | null> {
return await this.screenshotOnError(async () => {
return await this.underlyingPage.$(selector);
});
Expand Down Expand Up @@ -167,12 +170,17 @@ export class Page {
selector: string,
options?: WaitForSelectorOptions,
): Promise<string> {
if (options?.state === 'hidden' || options?.state === 'detached') {
throw new Error("Cannot get properties about selectors that shouldn't be there");
}
return await this.screenshotOnError(async () => {
const element = await this.underlyingPage.waitForSelector(selector, {
timeout: DEFAULT_PAGE_ELEMENT_WAIT_TIMEOUT_MS,
...options,
});
return await this.underlyingPage.evaluate(el => el.outerHTML, element);
// element! is safe because we verified that options.state is not one of the options
// that can result in waitForSelector returning null
return await this.underlyingPage.evaluate(el => el.outerHTML, element!);
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export async function scanForAccessibilityIssues(
async function injectAxeIfUndefined(page: Page): Promise<void> {
const axeIsUndefined = await page.evaluate(() => {
return (window as any).axe === undefined;
});
}, null);

if (axeIsUndefined) {
await page.injectScriptFile(
Expand Down
12 changes: 8 additions & 4 deletions tsconfig.strictNullChecks.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,9 @@
"./src/DetailsView/components/switcher-style-names.ts",
"./src/DetailsView/components/target-page-hidden-bar.tsx",
"./src/DetailsView/components/test-status-choice-group.tsx",
"./src/Devtools/inspect-handler.ts",
"./src/Devtools/target-page-inspector.ts",
"./src/ad-hoc-visualizations/issues/get-notification-message.ts",
"./src/assessments/adaptable-content/test-steps/test-step.ts",
"./src/assessments/assessment-default-message-generator.tsx",
"./src/assessments/audio-video-only/test-steps/test-steps.ts",
"./src/assessments/auto-pass-if-no-results.ts",
"./src/assessments/color/test-steps/test-steps.tsx",
Expand Down Expand Up @@ -112,6 +111,7 @@
"./src/common/components/expand-collapse-left-nav-hamburger-button.tsx",
"./src/common/components/external-link.tsx",
"./src/common/components/fix-instruction-processor.tsx",
"./src/common/components/flagged-component.tsx",
"./src/common/components/header-icon.tsx",
"./src/common/components/header.tsx",
"./src/common/components/issue-filing-needs-settings-help-text.tsx",
Expand Down Expand Up @@ -150,8 +150,7 @@
"./src/common/navigator-utils.ts",
"./src/common/platform.ts",
"./src/common/state-dispatcher.ts",
"./src/common/stores/client-stores-hub.ts",
"./src/common/stores/store-names.ts",
"./src/common/store-proxy.ts",
"./src/common/tabbable-elements-helper.ts",
"./src/common/types/deep-partial.ts",
"./src/common/types/details-view-pivot-type.ts",
Expand Down Expand Up @@ -333,6 +332,9 @@
"./src/tests/end-to-end/common/element-identifiers/target-page-selectors.ts",
"./src/tests/end-to-end/common/force-test-failure.ts",
"./src/tests/end-to-end/common/manifest-overide.ts",
"./src/tests/end-to-end/common/page-controllers/content-page.ts",
"./src/tests/end-to-end/common/page-controllers/page.ts",
"./src/tests/end-to-end/common/page-controllers/popup-page.ts",
"./src/tests/end-to-end/common/playwright-option-types.ts",
"./src/tests/end-to-end/common/prepare-test-result-file-path.ts",
"./src/tests/end-to-end/common/pretty-print-axe-violations.ts",
Expand Down Expand Up @@ -366,6 +368,7 @@
"./src/views/content/guidance-title.tsx"
],
"include": [
"src/Devtools/**/*",
"src/assessments/language/common/**/*",
"src/background/injector/**/*",
"src/common/action/**/*",
Expand All @@ -378,6 +381,7 @@
"src/common/message-creators/types/**/*",
"src/common/promises/**/*",
"src/common/react/**/*",
"src/common/stores/**/*",
"src/common/styles/**/*",
"src/common/types/property-bag/**/*",
"src/content/settings/**/*",
Expand Down

0 comments on commit c7a56b1

Please sign in to comment.