fix stale file in upload-progress event

[mifi]

fixes #4593

also fix some related types

[github-actions]

Diff output files

diff --git a/packages/@uppy/aws-s3-multipart/lib/index.js b/packages/@uppy/aws-s3-multipart/lib/index.js
index 742f01e..46efdda 100644
--- a/packages/@uppy/aws-s3-multipart/lib/index.js
+++ b/packages/@uppy/aws-s3-multipart/lib/index.js
@@ -1001,7 +1001,7 @@ function _uploadLocalFile2(file) {
   var _this = this;
   return new Promise((resolve, reject) => {
     const onProgress = (bytesUploaded, bytesTotal) => {
-      this.uppy.emit("upload-progress", file, {
+      this.uppy.emitProgressWithFile(file, {
         uploader: this,
         bytesUploaded,
         bytesTotal,
diff --git a/packages/@uppy/aws-s3/lib/MiniXHRUpload.js b/packages/@uppy/aws-s3/lib/MiniXHRUpload.js
index 0763aa7..52b5707 100644
--- a/packages/@uppy/aws-s3/lib/MiniXHRUpload.js
+++ b/packages/@uppy/aws-s3/lib/MiniXHRUpload.js
@@ -107,7 +107,7 @@ export default class MiniXHRUpload {
         this.uppy.log(`[AwsS3/XHRUpload] ${id} progress: ${ev.loaded} / ${ev.total}`);
         timer.progress();
         if (ev.lengthComputable) {
-          this.uppy.emit("upload-progress", file, {
+          this.uppy.emitProgressWithFile(file, {
             uploader: this,
             bytesUploaded: ev.loaded,
             bytesTotal: ev.total,
diff --git a/packages/@uppy/core/lib/Uppy.js b/packages/@uppy/core/lib/Uppy.js
index 9cf6978..ef5809d 100644
--- a/packages/@uppy/core/lib/Uppy.js
+++ b/packages/@uppy/core/lib/Uppy.js
@@ -227,6 +227,10 @@ export class Uppy {
     }
     _classPrivateFieldLooseBase(this, _emitter)[_emitter].emit(event, ...args);
   }
+  emitProgressWithFile(file, progress) {
+    const file2 = this.getFile(file.id);
+    this.emit("upload-progress", file2, progress);
+  }
   on(event, callback) {
     _classPrivateFieldLooseBase(this, _emitter)[_emitter].on(event, callback);
     return this;
@@ -1105,6 +1109,13 @@ function _transformFile2(fileDescriptorOrFile) {
   meta.name = fileName;
   meta.type = fileType;
   const size = Number.isFinite(file.data.size) ? file.data.size : null;
+  const progress = {
+    percentage: 0,
+    bytesUploaded: 0,
+    bytesTotal: size,
+    uploadComplete: false,
+    uploadStarted: null,
+  };
   return {
     source: file.source || "",
     id,
@@ -1116,13 +1127,7 @@ function _transformFile2(fileDescriptorOrFile) {
     },
     type: fileType,
     data: file.data,
-    progress: {
-      percentage: 0,
-      bytesUploaded: 0,
-      bytesTotal: size,
-      uploadComplete: false,
-      uploadStarted: null,
-    },
+    progress,
     size,
     isGhost: false,
     isRemote: file.isRemote || false,
@@ -1283,7 +1288,6 @@ function _addListeners2() {
     });
     const filesState = Object.fromEntries(filesFiltered.map(file => [file.id, {
       progress: {
-        progress: 0,
         uploadStarted: Date.now(),
         uploadComplete: false,
         percentage: 0,
diff --git a/packages/@uppy/tus/lib/index.js b/packages/@uppy/tus/lib/index.js
index c20af75..e499268 100644
--- a/packages/@uppy/tus/lib/index.js
+++ b/packages/@uppy/tus/lib/index.js
@@ -244,7 +244,7 @@ function _uploadLocalFile2(file) {
       if (typeof opts.onProgress === "function") {
         opts.onProgress(bytesUploaded, bytesTotal);
       }
-      this.uppy.emit("upload-progress", file, {
+      this.uppy.emitProgressWithFile(file, {
         uploader: this,
         bytesUploaded,
         bytesTotal,
diff --git a/packages/@uppy/utils/lib/emitSocketProgress.js b/packages/@uppy/utils/lib/emitSocketProgress.js
index f1e62a9..1af3916 100644
--- a/packages/@uppy/utils/lib/emitSocketProgress.js
+++ b/packages/@uppy/utils/lib/emitSocketProgress.js
@@ -7,7 +7,7 @@ function emitSocketProgress(uploader, progressData, file) {
   } = progressData;
   if (progress) {
     uploader.uppy.log(`Upload progress: ${progress}`);
-    uploader.uppy.emit("upload-progress", file, {
+    uploader.uppy.emitProgressWithFile(file, {
       uploader,
       bytesUploaded,
       bytesTotal,
diff --git a/packages/@uppy/utils/lib/getBytesRemaining.js b/packages/@uppy/utils/lib/getBytesRemaining.js
index 8a3ff8a..7c09b31 100644
--- a/packages/@uppy/utils/lib/getBytesRemaining.js
+++ b/packages/@uppy/utils/lib/getBytesRemaining.js
@@ -1,3 +1,4 @@
 export default function getBytesRemaining(fileProgress) {
+  if (fileProgress.bytesTotal == null) return 0;
   return fileProgress.bytesTotal - fileProgress.bytesUploaded;
 }
diff --git a/packages/@uppy/xhr-upload/lib/index.js b/packages/@uppy/xhr-upload/lib/index.js
index bb39afe..496f7d5 100644
--- a/packages/@uppy/xhr-upload/lib/index.js
+++ b/packages/@uppy/xhr-upload/lib/index.js
@@ -258,7 +258,7 @@ async function _uploadLocalFile2(file, current, total) {
       this.uppy.log(`[XHRUpload] ${id} progress: ${ev.loaded} / ${ev.total}`);
       timer.progress();
       if (ev.lengthComputable) {
-        this.uppy.emit("upload-progress", file, {
+        this.uppy.emitProgressWithFile(file, {
           uploader: this,
           uploadStarted,
           bytesUploaded: ev.loaded,
@@ -374,7 +374,7 @@ function _uploadBundle2(files) {
       timer.progress();
       if (!ev.lengthComputable) return;
       files.forEach(file => {
-        this.uppy.emit("upload-progress", file, {
+        this.uppy.emitProgressWithFile(file, {
           uploader: this,
           uploadStarted,
           bytesUploaded: ev.loaded / ev.total * file.size,

[Murderlon]

Isn't this fixing the symptom while the real fix is deep merging state instead of shallow merge?

If that's the case, I'd rather investigate that. If that's not the cause, I would prefer heading into the #3248 direction. So doing something like this:

this.emit('internal:upload-progress', file)

// in core
this.on('internal:upload-progress', (file) => {
    const file2 = this.getFile(file.id)
    this.emit('upload-progress', file2, progress)
})

[mifi]

No, see #4593 (comment) - I believe we are already deep cloning the file when progress changes (or any other change), and that is the reason why the old file reference becomes stale - it refers to the old uncloned file with its stale progress. If we were to remove the deep cloning, then it would work without this pr, but i believe immutable data store is a good thing so i dont think we should remove that.

[mifi]

isn't changing the name of the event a breaking change?

[mifi]

Suggested change

	emitProgressWithFile(
	protected emitProgressWithFile(

[mifi]

oops i misunderstood, you want to add an additional event internal:upload-progress, right? to replace the method call? I think an event adds complexity when all we need is simply a method (and nobody else needs this internal event). and it could lead to synchronousity issues due to the node.js event loop

[mifi]

I think we should refactor all events to include an up-to-date file object. maybe an emitWithFile method instead of emitProgressWithFile, for the events that also include the file object, that takes care of fetching the up-to-date file. and probably don't allow plugins to directly call this.uppy.emit() with these events, in order to reduce the risk of accidentally emitting stale files.

[aduh95]

and it could lead to synchronousity issues due to the node.js event loop

Node.js is not involved though, is it? I guess browsers event loop should be the same.

[aduh95]

Should we keep the as

[aduh95]

Superseded by #4928