Skip to content

Commit

Permalink
ImageLoader: fix types and remove @ts-expect-error instances (#1474)
Browse files Browse the repository at this point in the history
## Summary:

While chasing down a question from LearningEquality, I noticed some `@ts-expect-error` usages that made me wonder. After removing them I found it was quite easy to make the types correct.

This is also a common problem in our codebase following the port from Flow to TypeScript. We often set props to have the type `SomeType | null | undefined`. However, often we don't need to support it being `null`. In this case, removing the assignment of a default value (`null`) fixed the main issue.

Issue: "none"

## Test plan:

`yarn typecheck`
`yarn lint`

Author: jeremywiebe

Reviewers: jeremywiebe, mark-fitzgerald, benchristel, SonicScrewdriver, nishasy, catandthemachines, anakaren-rojas

Required Reviewers:

Approved By: mark-fitzgerald

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1474
  • Loading branch information
jeremywiebe authored Aug 1, 2024
1 parent eb9f3f9 commit a8aaac3
Show file tree
Hide file tree
Showing 5 changed files with 9 additions and 16 deletions.
5 changes: 5 additions & 0 deletions .changeset/blue-moons-carry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

Minor type fixes in the `ImageLoader` and `SvgImage` components
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ export const SvgImage = (args: StoryArgs): React.ReactElement => {
return (
<ImageLoader
src={svgUrl}
// @ts-expect-error [FEI-5003] - TS2322 - Type 'null' is not assignable to type '() => ReactElement<any, string | JSXElementConstructor<any>> | null | undefined'.
preloader={null}
imgProps={{
alt: "ALT",
Expand All @@ -34,7 +33,6 @@ export const PngImage = (args: StoryArgs): React.ReactElement => {
return (
<ImageLoader
src={imgUrl}
// @ts-expect-error [FEI-5003] - TS2322 - Type 'null' is not assignable to type '() => ReactElement<any, string | JSXElementConstructor<any>> | null | undefined'.
preloader={null}
imgProps={{
alt: "ALT",
Expand All @@ -50,7 +48,6 @@ export const InvalidImageWithChildrenForFailedLoading = (
return (
<ImageLoader
src="http://abcdefiahofshiaof.noway.badimage.com"
// @ts-expect-error [FEI-5003] - TS2322 - Type 'null' is not assignable to type '() => ReactElement<any, string | JSXElementConstructor<any>> | null | undefined'.
preloader={null}
imgProps={{
alt: "ALT",
Expand Down
12 changes: 4 additions & 8 deletions packages/perseus/src/components/image-loader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export type ImageProps = {
alt: string;
title?: string;
["aria-hidden"]?: boolean;
tabIndex?: string;
tabIndex?: number;
onClick?: (e: React.SyntheticEvent) => void;
style?: Dimensions;
};
Expand All @@ -41,7 +41,7 @@ type Props = {
// When the DOM updates to replace the preloader with the image, or
// vice-versa, we trigger this callback.
onUpdate: (status: (typeof Status)[keyof typeof Status]) => void;
preloader: () => React.ReactElement | null | undefined;
preloader: (() => React.ReactNode) | null | undefined;
src: string;
};

Expand Down Expand Up @@ -122,17 +122,15 @@ class ImageLoader extends React.Component<Props, State> {

renderImg: () => React.ReactElement<React.ComponentProps<"img">> = () => {
const {src, imgProps} = this.props;
let onKeyUp = null;
let onKeyDown = null;
let onKeyUp;
let onKeyDown;
if (imgProps.onClick != null) {
// @ts-expect-error - TS2322 - Type '(e: React.KeyboardEvent) => void' is not assignable to type 'null'.
onKeyUp = (e: React.KeyboardEvent) => {
// 13 is enter key, 32 is space key
if (e.keyCode === 13 || e.keyCode === 32) {
imgProps.onClick && imgProps.onClick(e);
}
};
// @ts-expect-error - TS2322 - Type '(e: React.KeyboardEvent) => void' is not assignable to type 'null'.
onKeyDown = (e: React.KeyboardEvent) => {
// 32 is space key
if (e.keyCode === 32) {
Expand All @@ -146,9 +144,7 @@ class ImageLoader extends React.Component<Props, State> {
return (
<img
src={staticUrl(src)}
// @ts-expect-error - TS2322 - Type 'null' is not assignable to type 'KeyboardEventHandler<HTMLImageElement> | undefined'.
onKeyUp={onKeyUp}
// @ts-expect-error - TS2322 - Type 'null' is not assignable to type 'KeyboardEventHandler<HTMLImageElement> | undefined'.
onKeyDown={onKeyDown}
{...imgProps}
/>
Expand Down
4 changes: 0 additions & 4 deletions packages/perseus/src/components/svg-image.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,6 @@ class SvgImage extends React.Component<Props, State> {
<ImageLoader
src={imageSrc}
imgProps={imageProps}
// @ts-expect-error - TS2322 - Type '(() => Element) | null' is not assignable to type '() => ReactElement<any, string | JSXElementConstructor<any>> | null | undefined'.
preloader={preloader}
onUpdate={this.handleUpdate}
/>
Expand All @@ -679,7 +678,6 @@ class SvgImage extends React.Component<Props, State> {
return (
<ImageLoader
src={imageSrc}
// @ts-expect-error - TS2322 - Type '(() => Element) | null' is not assignable to type '() => ReactElement<any, string | JSXElementConstructor<any>> | null | undefined'.
preloader={preloader}
imgProps={imageProps}
onUpdate={this.handleUpdate}
Expand Down Expand Up @@ -742,7 +740,6 @@ class SvgImage extends React.Component<Props, State> {
src={imageUrl}
onLoad={this.onImageLoad}
onUpdate={this.handleUpdate}
// @ts-expect-error - TS2322 - Type '(() => Element) | null' is not assignable to type '() => ReactElement<any, string | JSXElementConstructor<any>> | null | undefined'.
preloader={preloader}
imgProps={imageProps}
/>
Expand All @@ -758,7 +755,6 @@ class SvgImage extends React.Component<Props, State> {
src={imageUrl}
onLoad={this.onImageLoad}
onUpdate={this.handleUpdate}
// @ts-expect-error - TS2322 - Type '(() => Element) | null' is not assignable to type '() => ReactElement<any, string | JSXElementConstructor<any>> | null | undefined'.
preloader={preloader}
imgProps={imageProps}
/>
Expand Down
1 change: 0 additions & 1 deletion packages/perseus/src/server-item-renderer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ type SerializedState = {
hints: any;
};

/* eslint-disable-next-line react/no-unsafe */
export class ServerItemRenderer
extends React.Component<Props, State>
implements RendererInterface, KeypadContextRendererInterface
Expand Down

0 comments on commit a8aaac3

Please sign in to comment.