@uppy/status-bar: refactor to typescript

[aduh95]

No description provided.

[mifi]

Suggested change

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

to avoid NaN

[aduh95]

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

[arturi]

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.

[mifi]

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

[aduh95]

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

[mifi]

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.

[aduh95]

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.

[mifi]

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} ` : ''}

[aduh95]

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.

[mifi]

not sure what's the purpose of this

[aduh95]

It's to force TS to validate the object to be compatible with the defined type.

[Murderlon]

Overall looks good. There are some eslint warnings which would be nice to fix.

[mifi]

force push messed up my local branch :/

[mifi]

Suggested change

	(pv, { progress }) => pv + (progress.bytesUploaded as number),
	(pv, { progress }) => pv + (progress.bytesUploaded === false ? 0 : progress.bytesUploaded),

[mifi]

alternatively

Suggested change

	(pv, { progress }) => pv + (progress.bytesUploaded as number),
	(pv, { progress }) => pv + (Number(progress.bytesUploaded)),

[github-actions]

Diff output files

diff --git a/packages/@uppy/status-bar/lib/Components.js b/packages/@uppy/status-bar/lib/Components.js
index 77f7929..3f867a0 100644
--- a/packages/@uppy/status-bar/lib/Components.js
+++ b/packages/@uppy/status-bar/lib/Components.js
@@ -133,14 +133,16 @@ function PauseResumeButton(props) {
   } = props;
   const title = isAllPaused ? i18n("resume") : i18n("pause");
   function togglePauseResume() {
-    if (isAllComplete) return null;
+    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 h(
     "button",
diff --git a/packages/@uppy/status-bar/lib/StatusBar.js b/packages/@uppy/status-bar/lib/StatusBar.js
index 28c2148..a0656ee 100644
--- a/packages/@uppy/status-bar/lib/StatusBar.js
+++ b/packages/@uppy/status-bar/lib/StatusBar.js
@@ -38,12 +38,10 @@ function getUploadingState(error, isAllComplete, recoveredState, files) {
     if (progress.uploadStarted && !progress.uploadComplete) {
       return statusBarStates.STATE_UPLOADING;
     }
-    if (progress.preprocess && state !== statusBarStates.STATE_UPLOADING) {
+    if (progress.preprocess) {
       state = statusBarStates.STATE_PREPROCESSING;
     }
-    if (
-      progress.postprocess && state !== statusBarStates.STATE_UPLOADING && state !== statusBarStates.STATE_PREPROCESSING
-    ) {
+    if (progress.postprocess && state !== statusBarStates.STATE_PREPROCESSING) {
       state = statusBarStates.STATE_POSTPROCESSING;
     }
   }
diff --git a/packages/@uppy/status-bar/lib/StatusBarUI.js b/packages/@uppy/status-bar/lib/StatusBarUI.js
index 933ed3a..ea8eba7 100644
--- a/packages/@uppy/status-bar/lib/StatusBarUI.js
+++ b/packages/@uppy/status-bar/lib/StatusBarUI.js
@@ -137,8 +137,8 @@ export default function StatusBar(props) {
       role: "progressbar",
       "aria-label": `${width}%`,
       "aria-valuetext": `${width}%`,
-      "aria-valuemin": "0",
-      "aria-valuemax": "100",
+      "aria-valuemin": 0,
+      "aria-valuemax": 100,
       "aria-valuenow": progressValue,
     }),
     (() => {
@@ -226,3 +226,12 @@ export default function StatusBar(props) {
     ),
   );
 }
+StatusBar.defaultProps = {
+  doneButtonHandler: undefined,
+  hideAfterFinish: false,
+  hideCancelButton: false,
+  hidePauseResumeButton: false,
+  hideRetryButton: false,
+  hideUploadButton: undefined,
+  showProgressDetails: undefined,
+};
diff --git a/packages/@uppy/status-bar/lib/calculateProcessingProgress.js b/packages/@uppy/status-bar/lib/calculateProcessingProgress.js
index 5c71cd4..52dae93 100644
--- a/packages/@uppy/status-bar/lib/calculateProcessingProgress.js
+++ b/packages/@uppy/status-bar/lib/calculateProcessingProgress.js
@@ -1,6 +1,6 @@
 export default function calculateProcessingProgress(files) {
   const values = [];
-  let mode;
+  let mode = "indeterminate";
   let message;
   for (
     const {

[aduh95]

force push messed up my local branch :/

You can't mess up your local branches if you don't have local branches