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

ImageLoader: fix types and remove @ts-expect-error instances #1474

Merged
merged 3 commits into from
Aug 1, 2024

Conversation

jeremywiebe
Copy link
Collaborator

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

@jeremywiebe jeremywiebe self-assigned this Aug 1, 2024
@@ -28,7 +28,7 @@ export type ImageProps = {
alt: string;
title?: string;
["aria-hidden"]?: boolean;
tabIndex?: string;
tabIndex?: number;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This type error was exposed once I removed the @ts-expect-errors.

@@ -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;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main fix here is wrapping the function that returns a React element, otherwise it's a function that returns React.ReactElement | null | undefined. I changed the return type to React.ReactNode as that is what a render function returns.

Copy link
Contributor

github-actions bot commented Aug 1, 2024

Size Change: -111 B (-0.01%)

Total Size: 852 kB

Filename Size Change
packages/perseus/dist/es/index.js 413 kB -111 B (-0.03%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.3 kB
packages/kmath/dist/es/index.js 4.27 kB
packages/math-input/dist/es/index.js 80.8 kB
packages/math-input/dist/es/strings.js 1.73 kB
packages/perseus-core/dist/es/index.js 1.48 kB
packages/perseus-editor/dist/es/index.js 272 kB
packages/perseus-linter/dist/es/index.js 21.6 kB
packages/perseus/dist/es/strings.js 3.23 kB
packages/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

@@ -72,7 +72,6 @@ type SerializedState = {
hints: any;
};

/* eslint-disable-next-line react/no-unsafe */
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was not actually suppressing anything so I'm removing it.

@jeremywiebe jeremywiebe marked this pull request as ready for review August 1, 2024 21:17
@khan-actions-bot
Copy link
Contributor

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/blue-moons-carry.md, packages/perseus/src/server-item-renderer.tsx, packages/perseus/src/components/image-loader.tsx, packages/perseus/src/components/svg-image.tsx, packages/perseus/src/components/__stories__/image-loader.stories.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

Copy link
Contributor

github-actions bot commented Aug 1, 2024

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (a8e5a43) and published it to npm. You
can install it using the tag PR1474.

Example:

yarn add @khanacademy/perseus@PR1474

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1474

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.47%. Comparing base (eb9f3f9) to head (a8e5a43).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1474      +/-   ##
==========================================
+ Coverage   70.22%   70.47%   +0.24%     
==========================================
  Files         509      512       +3     
  Lines      105444   105473      +29     
  Branches     7631    11409    +3778     
==========================================
+ Hits        74049    74329     +280     
+ Misses      31212    31144      -68     
+ Partials      183        0     -183     

Impacted file tree graph

Files Coverage Δ
packages/perseus/src/components/image-loader.tsx 84.97% <100.00%> (+1.35%) ⬆️
packages/perseus/src/components/svg-image.tsx 65.05% <ø> (+3.45%) ⬆️
packages/perseus/src/server-item-renderer.tsx 83.60% <ø> (-0.04%) ⬇️

... and 141 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb9f3f9...a8e5a43. Read the comment docs.

Copy link
Contributor

@mark-fitzgerald mark-fitzgerald left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning these up.

@jeremywiebe jeremywiebe merged commit a8aaac3 into main Aug 1, 2024
11 checks passed
@jeremywiebe jeremywiebe deleted the jer/fix-types branch August 1, 2024 22:54
@jeremywiebe jeremywiebe mentioned this pull request Aug 1, 2024
jeremywiebe added a commit that referenced this pull request Aug 6, 2024
## Summary:

This PR contains several more type fixes following #1474:

  * `ImageUploader` deduped and parameter type fixed
  * Some stories moved to modern Storybook types (allowing us to eliminate some manual component prop type declarations). See [docs](https://storybook.js.org/docs/writing-stories#defining-stories).
  * `question` type on ItemEditor properly typed as `PerseusRenderer` type

Issue: "none"

## Test plan:

`yarn lint`
`yarn typecheck`

Author: jeremywiebe

Reviewers: jeremywiebe, mark-fitzgerald

Required Reviewers:

Approved By: mark-fitzgerald

Checks: ✅ codecov/project, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ codecov/patch, ✅ gerald, ⏭️  Publish npm snapshot, ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️  Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (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: #1476
Myranae pushed a commit that referenced this pull request Aug 9, 2024
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated.


# Releases
## @khanacademy/[email protected]

### Major Changes

-   [#1479](#1479) [`ef96cdd54`](ef96cdd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove PropCheckBox component from Perseus; use WB instead

### Minor Changes

-   [#1455](#1455) [`8b0268215`](8b02682) Thanks [@Myranae](https://github.com/Myranae)! - Update Polygon graphs to have weighted lines only on keyboard focus


-   [#1492](#1492) [`2ebbc1978`](2ebbc19) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Build the foundation for adding start coords UI for angle graphs


-   [#1428](#1428) [`eb9f3f9c0`](eb9f3f9) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Drop katex dependency - no longer used


-   [#1472](#1472) [`4c2ace57a`](4c2ace5) Thanks [@benchristel](https://github.com/benchristel)! - Add keyboard controls to Mafs angle graphs


-   [#1487](#1487) [`53031f8df`](53031f8) Thanks [@Myranae](https://github.com/Myranae)! - Make graphs non-interactive via keyboard when their question has been answered correctly


-   [#1486](#1486) [`0b625f560`](0b625f5) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for point graphs


-   [#1488](#1488) [`0bec013e8`](0bec013) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only)

### Patch Changes

-   [#1474](#1474) [`a8aaac339`](a8aaac3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Minor type fixes in the `ImageLoader` and `SvgImage` components


-   [#1493](#1493) [`de13fd337`](de13fd3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add/Edit/Delete Locked Function in Interactive Graph Editor


-   [#1476](#1476) [`18f38fca4`](18f38fc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Miscellaneous type improvements


-   [#1489](#1489) [`de2883b3f`](de2883b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Correct `hints` type on ItemRenderer


-   [#1478](#1478) [`7fd586ef4`](7fd586e) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Add "white" as a fill option for locked ellipses and polygons


-   [#1482](#1482) [`f920a4cc7`](f920a4c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor: Storybook] Add a story for Point graph type start coords


-   [#1484](#1484) [`808956098`](8089560) Thanks [@handeyeco](https://github.com/handeyeco)! - Minor cleanup around InfoTip


-   [#1475](#1475) [`6a218bcc1`](6a218bc) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - add non-visual text as a description for expression widget

-   Updated dependencies \[[`6c6ff52f4`](6c6ff52), [`342a72211`](342a722), [`5e66539e6`](5e66539)]:
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Minor Changes

-   [#1493](#1493) [`de13fd337`](de13fd3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add/Edit/Delete Locked Function in Interactive Graph Editor


-   [#1492](#1492) [`2ebbc1978`](2ebbc19) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Build the foundation for adding start coords UI for angle graphs


-   [#1486](#1486) [`0b625f560`](0b625f5) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for point graphs


-   [#1488](#1488) [`0bec013e8`](0bec013) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only)

### Patch Changes

-   [#1491](#1491) [`395b44f29`](395b44f) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Correct flag logic for polygon graph start coords UI


-   [#1476](#1476) [`18f38fca4`](18f38fc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Miscellaneous type improvements


-   [#1479](#1479) [`ef96cdd54`](ef96cdd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove PropCheckBox component from Perseus; use WB instead


-   [#1489](#1489) [`de2883b3f`](de2883b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Correct `hints` type on ItemRenderer


-   [#1478](#1478) [`7fd586ef4`](7fd586e) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Add "white" as a fill option for locked ellipses and polygons


-   [#1482](#1482) [`f920a4cc7`](f920a4c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor: Storybook] Add a story for Point graph type start coords


-   [#1471](#1471) [`b4e0b60dc`](b4e0b60) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Fix the dropdowns overlapping labels in locked figures settings


-   [#1481](#1481) [`a35be719f`](a35be71) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Stop page scrolling on number text field focused scroll


-   [#1484](#1484) [`808956098`](8089560) Thanks [@handeyeco](https://github.com/handeyeco)! - Minor cleanup around InfoTip


-   [#1477](#1477) [`653b520c6`](653b520) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Migrate ItemExtrasEditor to WB checkboxes

-   Updated dependencies \[[`a8aaac339`](a8aaac3), [`de13fd337`](de13fd3), [`8b0268215`](8b02682), [`2ebbc1978`](2ebbc19), [`6c6ff52f4`](6c6ff52), [`18f38fca4`](18f38fc), [`ef96cdd54`](ef96cdd), [`342a72211`](342a722), [`de2883b3f`](de2883b), [`7fd586ef4`](7fd586e), [`f920a4cc7`](f920a4c), [`eb9f3f9c0`](eb9f3f9), [`4c2ace57a`](4c2ace5), [`53031f8df`](53031f8), [`808956098`](8089560), [`5e66539e6`](5e66539), [`0b625f560`](0b625f5), [`6a218bcc1`](6a218bc), [`0bec013e8`](0bec013)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

-   [#1495](#1495) [`6c6ff52f4`](6c6ff52) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove old buttons that we weren't using anymore


-   [#1475](#1475) [`342a72211`](342a722) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - update wonder blocks popover versions


-   [#1496](#1496) [`5e66539e6`](5e66539) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove unused buttons from MathInput; add Lato

## @khanacademy/[email protected]

### Patch Changes

-   Updated dependencies \[[`6c6ff52f4`](6c6ff52), [`342a72211`](342a722), [`5e66539e6`](5e66539)]:
    -   @khanacademy/[email protected]

Author: khan-actions-bot

Reviewers: Myranae

Required Reviewers:

Approved By: Myranae

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

Pull Request URL: #1473
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.

3 participants