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.

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

Support for native files on Apache Cordova #134

Closed
wants to merge 16 commits into from

Conversation

Projects
None yet
4 participants
@hannuniemela
Copy link

commented Nov 19, 2018

Original idea was proposed about year ago, but PR was not completed. In Cordova FileReader onload event is received after read operation has successfully completed. Original proposition was to use Promise callback, but reviewers did not like Promises because compatibility issues with older browsers or possible polyfill dependency. PR contains old-style passed-in callbacks in slice function.

Additionally whether application is running in cordova is checked window has a property named cordova. The cordova.js file will add the cordova object in the global scope (the window).

@Acconut
Copy link
Member

left a comment

Thank you very much for your contribution. I left a few comments about your code. Besides that it would be nice if you could remove the changes to package-lock.json from this PR as they don't seem related to Cordova support.

Show resolved Hide resolved lib/browser/source.js Outdated
Show resolved Hide resolved lib/browser/source.js Outdated
Show resolved Hide resolved lib/upload.js Outdated
@hannuniemela

This comment has been minimized.

Copy link
Author

commented Nov 20, 2018

Travis found also some issues to correct. Do you notice anything else or are we happy with cordova additions now. At least it seems to work. Brilliant!

@Acconut

This comment has been minimized.

Copy link
Member

commented Nov 25, 2018

Thank you very much for the changes, they all look good! We currently have another big PR (#126) which also works on the same parts as you did, so we would like to merge that one before. I hope that we can merge it next week and then we can fully integrate your patches into tus-js-client.

@Acconut

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

I merged some changes from our master branch into this PR, which would be necessary for completing this. However, I did some more research and I think there is already an easier way to use tus-js-client in Cordova apps:

The FileEntry objects have a file method (https://cordova.apache.org/docs/en/1.8.0/cordova/file/fileentry/fileentry.html#methods and https://github.com/apache/cordova-plugin-file/blob/5e12b5e9f63b12aef5198e9a72ac8d7b8e834442/www/FileEntry.js#L80) which can be used to obtain a File instance asynchronously. This File instance is similar to the ones in browsers as it has the slice(start, end) method (https://github.com/apache/cordova-plugin-file/blob/5e12b5e9f63b12aef5198e9a72ac8d7b8e834442/www/File.js#L52), so tus-js-client should be able to deal with them.

Did you try this approach yet? I would prefer that one since it adds less moving parts to tus-js-client which would make it harder to maintain.

@hannuniemela

This comment has been minimized.

Copy link
Author

commented Dec 10, 2018

cordova-plugin-file is required to when accessing files in Cordova application, and is presumed also when using tus-js-client. In PR CordovaFileSource class encapsulates reading slice of File instance. File chunk is read using readAsArrayBuffer function of Cordova FileReader. After successful read is completed onload event called, where reader result as a binary data is forwarded using callback in PR. So it requires FileReader to access File and read chunk of file to binary data. I could reference here a good sample of reading file in chunks in Cordova (the last code block of web page) in https://clockwise.software/blog/dealing-with-large-files-in-cordova/

@hannuniemela

This comment has been minimized.

Copy link
Author

commented Dec 12, 2018

I made some changes by encapsulating Cordova's FileReader to separate file. It takes file.slice result as a parameter and returns byte array as a callback. Maybe this solution is nicer than first tryout. What do you think?

if (isCordova()) {
readAsByteArray(chunk, (error, byteArray) => {
if (error) {
this._emitError(new DetailedError(`tus: could not read file (from ${start} to ${end})`, error));

This comment has been minimized.

Copy link
@kvz

kvz Dec 13, 2018

Member

Should we still xhr.send in the face of this error or is there a missing return?

This comment has been minimized.

Copy link
@hannuniemela

hannuniemela Dec 13, 2018

Author

Moved that block to source.js in commit 7380fdb since that would be more suitable Could you confirm that is correct. There should be this also corrected

This comment has been minimized.

Copy link
@Acconut

Acconut Dec 14, 2018

Member

It looks OK from what I can tell when looking at it from the git diff.

@Acconut

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

I made some changes by encapsulating Cordova's FileReader to separate file. It takes file.slice result as a parameter and returns byte array as a callback. Maybe this solution is nicer than first tryout. What do you think?

Frankly, I don't mind. The function is small enough that I am not too much concerned about its placement.

However, I wanted to suggest an alternative solution in my comment (#134 (comment)) to the code you propose in this PR. I don't want to talk it down but I am a bit conservative since we have to maintain every line that gets added to tus-js-client if you know what I mean :)

That's why I asked for your opinion on my proposal but I think that question remained unanswered. Would you mind having a look at it?

@hannuniemela

This comment has been minimized.

Copy link
Author

commented Dec 16, 2018

In Cordova FileReader object is required to read files from device's file system https://cordova.apache.org/docs/en/3.0.0/cordova/file/filereader/filereader.html Therefore I cannot figure out how to add Cordova support to tus-js-client without using that piece of code. Is it better or worse if reading a file chunk is externalized from tus-js-client by adding a optional "fileReader" callback to options and is called in slice function whether this callback is defined aka this "readAsByteArray"-like code is externalized from module. By that way these Cordova like systems (if there are other similar) could be supported and maintained system specific changed minimized. But I cannot see any other options to make tus-js-client work in Cordova since FileReader is required to access files.

@Acconut

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

In Cordova FileReader object is required to read files from device's file system https://cordova.apache.org/docs/en/3.0.0/cordova/file/filereader/filereader.html

This documentation seems to be outdated. What version of Cordova are you using?
FileReader may be the only way to read a file in order to access its content directly. But tus-js-client does not need to do that. Instead, it's enough to obtain references to file sections and then tell the environment to upload this file section without ever looking at its content. That's why we might not need FileReader at all.

Here, let me try to show you some code about what I wanted to describe (please keep in mind that I wrote this out of my head and hasn't been tested):

// You get this variable somewhere, probably your file picker.
var fileEntry = ...

fileEntry.file(function onSuccess(file) {
    // We just resolved the FileEntry object to a File instance which should be usable with tus-js-client:

    var upload = new tus.Upload(file, {
        endpoint: "http://localhost:1080/files/",
        retryDelays: [0, 1000, 3000, 5000],
        metadata: {
            filename: file.name,
            filetype: file.type
        },
        onError: function(error) {
            console.log("Failed because: " + error)
        },
        onProgress: function(bytesUploaded, bytesTotal) {
            var percentage = (bytesUploaded / bytesTotal * 100).toFixed(2)
            console.log(bytesUploaded, bytesTotal, percentage + "%")
        },
        onSuccess: function() {
            console.log("Download %s from %s", upload.file.name, upload.url)
        }
    })

    // Start the upload
    upload.start()
}, function onError(err) {
    // This callback is used when the file entry could not be resolved.
});

With this snipped, I hope, that you can use tus-js-client without any modifications on Cordova.

@naranjamecanica

This comment has been minimized.

Copy link

commented Dec 23, 2018

Hi, If I recall correctly, the problem was that the file object returned is not the same as in a browser, but a Cordova File object. In this object the slice method was not working as expected.

Thanks for your time to finetune the solution, looks good to me, I'll test it in mid January and put it in production on the 80+ iPads in a museum where we used TUS after that.

@kvz

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

Sounds like an interesting use case @naranjamecanica, maybe one we could write something about. Can I email you to learn more or not interested? 'no' is a perfectly fine answer ofc

@hannuniemela

This comment has been minimized.

Copy link
Author

commented Jan 2, 2019

Like @naranjamecanica and also I mentioned previously, slice method won't work on Cordova without FileReader. Previously I also referred kind of old version of Cordova FileReader but I am quite confident it is all same in current releases. Due we are using Cordova 6.x version is that we adopted TUS upload during a feature upgrade for application already in production.

@naranjamecanica

This comment has been minimized.

Copy link

commented Jan 9, 2019

Sounds like an interesting use case @naranjamecanica, maybe one we could write something about. Can I email you to learn more or not interested? 'no' is a perfectly fine answer ofc

Yes of course you can!

@Acconut

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

I built an example application with Apache Cordova on my own to see how it works. You can find it at https://github.com/tus/tus-js-client/tree/master/demos/cordova. To my surprise, I used the normal tus-js-client version (without patches from this PR) and did not ran into any problems. I am just using the file from a HTML file input element and pass it directly to tus.Upload.

So I am a bit confused to why we would need additional handling from this PR. Could this be a new feature in Cordova 8 (which I used for my testing)? Or are the problems that you were talking about only present on specific platforms (I tested my app on a real Android device)?

@naranjamecanica

This comment has been minimized.

Copy link

commented Jan 15, 2019

Ah, that explains the miscommunication, I never really used it in from a html form. In our project we load files from the local filesystem (captured media) using resolveLocalFileSystemURL which returns an fileEntry object, which allows you to retrieve a file object trough the file() method. This returned file object is not exactly the same as a browser version, so the thats why the proposed changes are needed.

In my pull request I checked the localUrl of the input file for the presence of cdvfile to make sure it's really a file object from the local file system and not proxied by the browser:

if (input && input.localURL && input.localURL.indexOf('cdvfile') === 0) {

Hope this clears everything up!

@Acconut

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Ah, thank you very much, @naranjamecanica. That explains my different experience with Cordova. Would one of you be able to supply an example of using resolveLocalFileSystemURL? That would be nice to test this PR and point future users somewhere if they need help.

@hannuniemela

This comment has been minimized.

Copy link
Author

commented Jan 19, 2019

I merged your Cordova demo @Acconut to my fork and changed it to use Photos library with Camera plugin and accessing native file system File plugin. The assumptions were correct that slice behaves differently with native File object than browser object. In my fork's master branch illustrates how native file is not uploading and cordova-file-support branch contains working demo.

Hannu Niemelä and others added some commits Jan 21, 2019

@Acconut Acconut changed the title cordova file support Support for native files on Apache Cordova Jan 22, 2019

Acconut added a commit that referenced this pull request Jan 22, 2019

Support uploading native files from Apache Cordova (#134)
This commit contains the changes from #134 and a few more minor improvements
@Acconut

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

@hannuniemela Thank you very much for your updates. It showed me how different Cordova's File is from the browser's File (what you have been correctly saying for the entire time). For a record, the problem is that Cordova's File is not a sub-type of Blob and therefore slices of a file cannot be passed to a XMLHttpRequest#send (when doing so, you will send the string [object Object] and not the file content).

Anyways, I have added a few more comments and fixed some styling issues before merging this PR in b926243.

You can expect your contribution to be in the next 1.6.0 release which I am about to publish right now. Finally, I want to thank you both again for your continued support and patience with me :)

@Acconut Acconut closed this Jan 22, 2019

@hannuniemela

This comment has been minimized.

Copy link
Author

commented Jan 23, 2019

Happy to be involved in with this project :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.