Fix File Progress Type #1140
Fix File Progress Type #1140
Conversation
|
Is there any issue to put the type at last position, for backward compatibility ? |
| @@ -159,27 +159,28 @@ const RESTController = { | |||
| headers[key] = customHeaders[key]; | |||
| } | |||
|
|
|||
| function handleProgress(event) { | |||
| function handleProgress(type, event) { | |||
| if (options && typeof options.progress === 'function') { | |||
| if (event.lengthComputable) { | |||
| options.progress( | |||
There was a problem hiding this comment.
I pushed it at the beginning, or
progress(event.loaded / event.total, {
type,
event
});@dplewis , @ladigitale let me know your thoughts if you have a free time 😄
There was a problem hiding this comment.
I do think there is. I was reluctant to just add it and the end, somehow, why not just enclose it in object to allow future values instead of
progress(progressValue, loaded, total, type, ...restArgs)
There was a problem hiding this comment.
it would be perfect.
In fact when we've got the original event i think we've got the type.
The targets of each event have a different type who gives the needed imformation.
There was a problem hiding this comment.
Is that so? Have you tried it? haven't looked into values of event tho.
There was a problem hiding this comment.
I just logged the target propety one is XMLHttpRequest, and the other is XMLHttpRequestUpload
There was a problem hiding this comment.
Feel free to make suggestion on the PR using the suggestion feature of Github. 😄
src/RESTController.js
Outdated
| type, | ||
| event.loaded / event.total, | ||
| event.loaded, | ||
| event.total |
There was a problem hiding this comment.
we could keep the 3 first parameters as is for backward compatibility, then we could add an object that can handle any future addition. we would just give the original event in it.
By testing original event.target people could infere if it's upload or download phase.
event.loaded / event.total,
event.loaded,
event.total,
{originalEvent:event}
But i'm ok with
event.loaded / event.total,
event.loaded,
event.total,
type
and with
event.loaded / event.total,
event.loaded,
event.total,
event
for example.
There was a problem hiding this comment.
for determining the type of progress, we can use event.target, right?
There was a problem hiding this comment.
Upon checking, I tried logging event and here's what I got:
I could check the instance, but it would be better to tell explicitly rather then leaving us the logic for checking whether it came from download or upload
There was a problem hiding this comment.
yes. I agree with that.
Maybe satisfying and scallable solution would be
event.loaded / event.total, event.loaded, event.total, {type:type}
as arguments of the progress function.
There was a problem hiding this comment.
@ladigitale I see what you are trying to do now. I didn't realize there was a difference. I agree on the backwards compatibility by adding the options to the end.
There are other places in the code that you can add progress for instance file.getData in Parse.File, FileController.download etc
Codecov Report
@@ Coverage Diff @@
## master #1140 +/- ##
=======================================
Coverage 92.30% 92.30%
=======================================
Files 54 54
Lines 5235 5235
Branches 1169 1169
=======================================
Hits 4832 4832
Misses 403 403
Continue to review full report at Codecov.
|
According to @ladigitale from last PR