Fix js error "The parameter is incorrect"

trongrg commented 5 years ago

Platforms affected

windows

What does this PR do?

Fix js error "The parameter is incorrect" when download progress is 0 in error handler

What testing has been done on this change?

Manual testing

Checklist

[ ] Reported an issue in the JIRA database
[ ] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
[ ] Added automated test coverage as appropriate for this change.

janpio commented 5 years ago

Hey @trongrg, thanks for this PR! Can you explain the motivation behind it a bit? When does this happen? What exactly is your code doing to fix it?

trongrg commented 5 years ago

hey @janpio

So I have an UWP app using imgcache.js https://github.com/chrisben/imgcache.js to cache the images. imgcache.js use this plugin to download images.

When we load a page with 40 images, some of the images are broken, and it jumps to the error handler getTransferError. In my case, it does have a response however the download.progress.bytesReceived is 0, and Windows.Storage.Streams.DataReader throw an exception complaining about "The parameter is incorrect" for that 0

So this PR check download.progress.bytesReceived, and if it is 0, resolve the error, so no exception is thrown

janpio commented 5 years ago

What does "download.progress.bytesReceived is 0" mean, in layman terms? Is it a good idea to just "swallow" that exception?

trongrg commented 5 years ago

"download.progress.bytesReceived is 0" means the image is downloaded, but on hard disk its size is 0. This could be the result of a broken transfer.

I didn't swallow the exception (it is hard to digest anyway). This exception was thrown by a bug in the code, not intentional.

The purpose of that piece of code is to resolve a FILE_NOT_FOUND_ERR, with the downloaded payload. So it try to load the downloaded bytes, however when the bytesReceived is 0, WinJS throws an exception, and break the callback

After the change, it still resolves a FILE_NOT_FOUND_ERR, just without a payload in case bytesReceived is 0

janpio commented 5 years ago

Ok, way above my pay grade ($0) - let's hope someone who knows a bit about this comes along to merge this.

The tests on Travis are failing right now:

1) cordova-plugin-file-transfer-tests.tests >> FileTransfer methods download filetransfer.spec.8 should download a file using https://
  - Expected spy httpFail not to have been called.
2) cordova-plugin-file-transfer-tests.tests >> FileTransfer methods download filetransfer.spec.9 should not leave partial file due to abort
  - Expected spy httpWin not to have been called.
3) cordova-plugin-file-transfer-tests.tests >> FileTransfer methods download filetransfer.spec.10 should be stopped by abort()
  - Expected spy httpWin not to have been called.

and

1) cordova-plugin-file-transfer-tests.tests >> FileTransfer methods download filetransfer.spec.9 should not leave partial file due to abort
  - Expected spy httpWin not to have been called.
2) cordova-plugin-file-transfer-tests.tests >> FileTransfer methods download filetransfer.spec.10 should be stopped by abort()
  - Expected spy httpWin not to have been called.

and

1) cordova-plugin-file-transfer-tests.tests >> FileTransfer methods download filetransfer.spec.9 should not leave partial file due to abort
  - Expected spy httpWin not to have been called.

Is this caused by these changes?

trongrg commented 5 years ago

I don't think this PR cause those to fail, because the change is for windows platform, and the tests are failing for android platform

PabbleDabble commented 5 years ago

@janpio Hello. Apologies if this is the wrong place to ask / if this is a bad question. I see the 1.7.1 release was release Jan of 2018, and the 1.7.0 updated the readme with deprecated. I also see there have been merges in to master more recently than Jan of 2018 (specifically this one). Does this plugin being deprecated mean there will no longer be releases? I know I'm able to download this plugin manually, but was hoping to keep this installation using the ionic cordova add method to stay consistent with our process. Thank you so much for any information.

janpio commented 5 years ago

This is definitely the wrong place, also asking individual committers is not standard. Please create a new issue with your question.

apache / cordova-plugin-file-transfer