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.

Sign up for GitHub

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

@uppy/status-bar: refactor to typescript #4839

Merged
merged 13 commits into from
Jan 22, 2024
Merged

@uppy/status-bar: refactor to typescript #4839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4d7893d
@uppy/status-bar: refactor to typescript
aduh95 Dec 27, 2023
8561abd
fixup
aduh95 Dec 29, 2023
45fac24
revert unrelated change
aduh95 Dec 29, 2023
c890cf6
fix TS build
aduh95 Dec 30, 2023
86cb254
fixup! fix TS build
aduh95 Dec 30, 2023
76d29f2
fix paths
aduh95 Dec 30, 2023
de3dd27
Apply suggestions from code review
aduh95 Dec 30, 2023
4aa00e8
Update packages/@uppy/status-bar/src/Components.tsx
aduh95 Jan 3, 2024
4cb1caf
Apply suggestions from code review
aduh95 Jan 16, 2024
4854bba
Apply suggestions from code review
aduh95 Jan 16, 2024
271f3da
Update packages/@uppy/status-bar/src/StatusBar.tsx
aduh95 Jan 17, 2024
e3a778d
Merge branch 'main' into status-bar-refactor
aduh95 Jan 19, 2024
7ac4402
mark opts as optional
aduh95 Jan 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 145 additions & 40 deletions packages/@uppy/status-bar/src/Components.jsx → packages/@uppy/status-bar/src/Components.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,30 @@
import type { Body, Meta } from '@uppy/utils/lib/UppyFile'
import type { State, Uppy } from '@uppy/core/src/Uppy.ts'
import type { FileProcessingInfo } from '@uppy/utils/lib/FileProgress'
import type { I18n } from '@uppy/utils/lib/Translator'
import { h } from 'preact'
import classNames from 'classnames'
import prettierBytes from '@transloadit/prettier-bytes'
import prettyETA from '@uppy/utils/lib/prettyETA'

import statusBarStates from './StatusBarStates.js'
import statusBarStates from './StatusBarStates.ts'

const DOT = `\u00B7`
const renderDot = () => ` ${DOT} `
const renderDot = (): string => ` ${DOT} `
mifi marked this conversation as resolved.
Show resolved Hide resolved

interface UploadBtnProps<M extends Meta, B extends Body> {
newFiles: number
isUploadStarted: boolean
recoveredState: null | State<M, B>
i18n: I18n
uploadState: string
isSomeGhost: boolean
startUpload: () => void
}

function UploadBtn (props) {
function UploadBtn<M extends Meta, B extends Body>(
props: UploadBtnProps<M, B>,
): JSX.Element {
const {
newFiles,
isUploadStarted,
Expand All @@ -30,9 +46,10 @@
{ 'uppy-StatusBar-actionBtn--disabled': isSomeGhost },
)

const uploadBtnText = newFiles && isUploadStarted && !recoveredState
? i18n('uploadXNewFiles', { smart_count: newFiles })
: i18n('uploadXFiles', { smart_count: newFiles })
const uploadBtnText =
newFiles && isUploadStarted && !recoveredState
? i18n('uploadXNewFiles', { smart_count: newFiles })
: i18n('uploadXFiles', { smart_count: newFiles })

return (
<button
Expand All @@ -48,15 +65,26 @@
)
}

function RetryBtn (props) {
interface RetryBtnProps<M extends Meta, B extends Body> {
i18n: I18n
uppy: Uppy<M, B>
}

function RetryBtn<M extends Meta, B extends Body>(
props: RetryBtnProps<M, B>,
): JSX.Element {
const { i18n, uppy } = props

return (
<button
type="button"
className="uppy-u-reset uppy-c-btn uppy-StatusBar-actionBtn uppy-StatusBar-actionBtn--retry"
aria-label={i18n('retryUpload')}
onClick={() => uppy.retryAll().catch(() => { /* Error reported and handled via an event */ })}
onClick={() =>
uppy.retryAll().catch(() => {
/* Error reported and handled via an event */
})
}
data-uppy-super-focusable
data-cy="retry"
>
Expand All @@ -75,7 +103,14 @@
)
}

function CancelBtn (props) {
interface CancelBtnProps<M extends Meta, B extends Body> {
i18n: I18n
uppy: Uppy<M, B>
}

function CancelBtn<M extends Meta, B extends Body>(
props: CancelBtnProps<M, B>,
): JSX.Element {
const { i18n, uppy } = props

return (
Expand All @@ -84,7 +119,7 @@
className="uppy-u-reset uppy-StatusBar-actionCircleBtn"
title={i18n('cancel')}
aria-label={i18n('cancel')}
onClick={() => uppy.cancelAll()}
onClick={(): void => uppy.cancelAll()}
data-cy="cancel"
data-uppy-super-focusable
>
Expand All @@ -108,22 +143,34 @@
)
}

function PauseResumeButton (props) {
interface PauseResumeButtonProps<M extends Meta, B extends Body> {
i18n: I18n
uppy: Uppy<M, B>
isAllPaused: boolean
isAllComplete: boolean
resumableUploads: boolean
}

function PauseResumeButton<M extends Meta, B extends Body>(
props: PauseResumeButtonProps<M, B>,
): JSX.Element {
const { isAllPaused, i18n, isAllComplete, resumableUploads, uppy } = props
const title = isAllPaused ? i18n('resume') : i18n('pause')

function togglePauseResume () {
if (isAllComplete) return null
function togglePauseResume(): void {
if (isAllComplete) return

if (!resumableUploads) {
return uppy.cancelAll()
uppy.cancelAll()
return
}

if (isAllPaused) {
return uppy.resumeAll()
uppy.resumeAll()
return
}

return uppy.pauseAll()
uppy.pauseAll()
}

return (
Expand Down Expand Up @@ -160,22 +207,27 @@
)
}

function DoneBtn (props) {
interface DoneBtnProps {
i18n: I18n
doneButtonHandler: (() => void) | null
}

function DoneBtn(props: DoneBtnProps): JSX.Element {
const { i18n, doneButtonHandler } = props

return (
<button
type="button"
className="uppy-u-reset uppy-c-btn uppy-StatusBar-actionBtn uppy-StatusBar-actionBtn--done"
onClick={doneButtonHandler}
onClick={doneButtonHandler!}

Check warning on line 222 in packages/@uppy/status-bar/src/Components.tsx

View workflow job for this annotation

GitHub Actions / Lint JavaScript/TypeScript 

Forbidden non-null assertion
mifi marked this conversation as resolved.
Show resolved Hide resolved
data-uppy-super-focusable
>
{i18n('done')}
</button>
)
}

function LoadingSpinner () {
function LoadingSpinner(): JSX.Element {
return (
<svg
className="uppy-StatusBar-spinner"
Expand All @@ -192,10 +244,14 @@
)
}

function ProgressBarProcessing (props) {
interface ProgressBarProcessingProps {
progress: FileProcessingInfo
}

function ProgressBarProcessing(props: ProgressBarProcessingProps): JSX.Element {
const { progress } = props
const { value, mode, message } = progress
const roundedValue = Math.round(value * 100)
const roundedValue = Math.round(value! * 100)

Check warning on line 254 in packages/@uppy/status-bar/src/Components.tsx

View workflow job for this annotation

GitHub Actions / Lint JavaScript/TypeScript 

Forbidden non-null assertion
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const roundedValue = Math.round(value! * 100)
const roundedValue = value != null ? Math.round(value * 100) : 0

to avoid NaN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is about adding types, we should refrain from making any change that would affect the JS output

Copy link
Contributor

Choose a reason for hiding this comment

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

But I thought that rule was to not introduce breaking changes. In this case, does it matter that the JS output changes? I think we’re missing on the critical benefit of a TS refactor, which is improving our code to be more stable along the way, like in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we fix be fixing bugs that we discover while refactoring to TS, even though it changes the JS output? At the very least we should add a TODO to fix such discovered bugs in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO non-null assertions are an implicit TODO. We should go through them and remove them on the 4.x branch

Copy link
Contributor

Choose a reason for hiding this comment

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

I think non-null assertions are fine to use in some cases, when the alternative is to check for nullish and throw an error (like maybeNull!.something). But in this case, the bug will render NaN, and in this case the correct solution is probably not to throw an error, but instead render something like 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uppy/packages/@uppy/status-bar/src/Components.tsx

Line 260 in 4854bba

{mode === 'determinate' ? `${roundedValue}% ${dot} ` : ''}

As you can see, if the value is indeterminate, that roundedValue will not be read.

Copy link
Contributor

@mifi mifi Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

then it's better to put the round after the determinate check, then we don't need the non-null assertion, because typescript already knows that it's not nullish:

      {mode === 'determinate' ? `${Math.round(value * 100)}% ${dot} ` : ''}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but I'd prefer to not introduce runtime changes when we can avoid it. My wish is we'd be able to get rid of all non-null assertions on the 4.x branch, and turn the linter warning to an error.

const dot = `\u00B7`

return (
Expand All @@ -207,22 +263,25 @@
)
}

function ProgressDetails (props) {
const {
numUploads,
complete,
totalUploadedSize,
totalSize,
totalETA,
i18n,
} = props
interface ProgressDetailsProps {
i18n: I18n
numUploads: number
complete: number
totalUploadedSize: number
totalSize: number
totalETA: number
}

function ProgressDetails(props: ProgressDetailsProps): JSX.Element {
const { numUploads, complete, totalUploadedSize, totalSize, totalETA, i18n } =
props

const ifShowFilesUploadedOfTotal = numUploads > 1

return (
<div className="uppy-StatusBar-statusSecondary">
{ifShowFilesUploadedOfTotal
&& i18n('filesUploadedOfTotal', {
{ifShowFilesUploadedOfTotal &&
i18n('filesUploadedOfTotal', {
complete,
smart_count: numUploads,
})}
Expand All @@ -248,7 +307,13 @@
)
}

function FileUploadCount (props) {
interface FileUploadCountProps {
i18n: I18n
complete: number
numUploads: number
}

function FileUploadCount(props: FileUploadCountProps): JSX.Element {
const { i18n, complete, numUploads } = props

return (
Expand All @@ -258,7 +323,13 @@
)
}

function UploadNewlyAddedFiles (props) {
interface UploadNewlyAddedFilesProps {
i18n: I18n
newFiles: number
startUpload: () => void
}

function UploadNewlyAddedFiles(props: UploadNewlyAddedFilesProps): JSX.Element {
const { i18n, newFiles, startUpload } = props
const uploadBtnClassNames = classNames(
'uppy-u-reset',
Expand All @@ -284,7 +355,26 @@
)
}

function ProgressBarUploading (props) {
interface ProgressBarUploadingProps {
i18n: I18n
supportsUploadProgress: boolean
totalProgress: number
showProgressDetails: boolean | undefined
isUploadStarted: boolean
isAllComplete: boolean
isAllPaused: boolean
newFiles: number
numUploads: number
complete: number
totalUploadedSize: number
totalSize: number
totalETA: number
startUpload: () => void
}

function ProgressBarUploading(
props: ProgressBarUploadingProps,
): JSX.Element | null {
const {
i18n,
supportsUploadProgress,
Expand All @@ -310,7 +400,7 @@

const title = isAllPaused ? i18n('paused') : i18n('uploading')

function renderProgressDetails () {
function renderProgressDetails(): JSX.Element | null {
if (!isAllPaused && !showUploadNewlyAddedFiles && showProgressDetails) {
if (supportsUploadProgress) {
return (
Expand Down Expand Up @@ -357,7 +447,11 @@
)
}

function ProgressBarComplete (props) {
interface ProgressBarCompleteProps {
i18n: I18n
}

function ProgressBarComplete(props: ProgressBarCompleteProps): JSX.Element {
const { i18n } = props

return (
Expand Down Expand Up @@ -385,10 +479,17 @@
)
}

function ProgressBarError (props) {
interface ProgressBarErrorProps {
i18n: I18n
error: any
complete: number
numUploads: number
}

function ProgressBarError(props: ProgressBarErrorProps): JSX.Element {
const { error, i18n, complete, numUploads } = props

function displayErrorAlert () {
function displayErrorAlert(): void {
const errorMessage = `${i18n('uploadFailed')} \n\n ${error}`
// eslint-disable-next-line no-alert
alert(errorMessage) // TODO: move to custom alert implementation
Expand Down Expand Up @@ -422,7 +523,11 @@
</button>
</div>

<FileUploadCount i18n={i18n} complete={complete} numUploads={numUploads} />
<FileUploadCount
i18n={i18n}
complete={complete}
numUploads={numUploads}
/>
</div>
</div>
)
Expand Down
Loading
Loading