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

Add support for React Native #132

Merged
merged 15 commits into from Dec 9, 2018

Conversation

Projects
None yet
2 participants
@arturi
Copy link
Contributor

commented Nov 15, 2018

This PR adds support for React Native environment (both Expo and regular React Native).

When in React Native environment, and a file object with a uri property is passed to tus.Upload, instead of a Blob that tus normally expects, we first fetch the blob from a local uri using XMLHttpRequest.

Hopefully fixes #96.

This is a draft PR, I realize things might need to be moved around or maybe reworked entirely. But it seems to be working fine locally for me, when passing tus.Upload({ url: 'file://...' }).

@Acconut
Copy link
Member

left a comment

Wow, very nice work, Artur! I have three topics I would like to talk to you about:

  1. I think this code better belongs somewhere else. In tus-js-client we have Sources (https://github.com/tus/tus-js-client/blob/master/lib/browser/source.js and https://github.com/tus/tus-js-client/blob/master/lib/node/source.js) which provide a unified abstraction above Files, Blobs and Streams in the browser and Node.js.This is also where the URI resolver should be. The basic idea would be to make the getSource (https://github.com/tus/tus-js-client/blob/master/lib/browser/source.js#L14) method asynchronous, such as:
export function getSource(input, callback) {
    // In React Native, when user selects a file, instead of a File or Blob, 
    // you usually get a file object {} with a uri property that contains
    // a local path to the file. We use XMLHttpRequest to fetch 
    // the file blob, before uploading with tus.
    if (isReactNative && 
      typeof file === 'object' && 
      file.url !== null) {
        return uriToBlob(file.uri).then((blob) => {
          callback(null, new FileSource(blob))
        })
        .catch((err) => {
          callback(new Error("tus: cannot fetch `file.uri` as Blob, make sure the uri is correct and accessible"));
        })
  }
  
  // Since we emulate the Blob type in our tests (not all target browsers
  // support it), we cannot use `instanceof` for testing whether the input value
  // can be handled. Instead, we simply check is the slice() function and the
  // size property are available.
  if (typeof input.slice === "function" && typeof input.size !== "undefined") {
    return callback(null, new FileSource(input));
  }

  throw new Error("source object may only be an instance of File or Blob in this environment");
}
  1. I would prefer it if the uriToBlob method would not use Promises. I know they may be easier to use but tus-js-client uses callbacks for its internals and the exposed interface and i would like to keep the library code consistent.

  2. It would be nice to have a test for this when the major work is done. I am not talking about a full system them where we launch an iOS/Android emulator and check the upload but rather about a simple implementation test to make sure that tus-js-client actually resolves an URI if we provide it with one. For that we have browser tests with a mocked XMLHttpRequest API to hook into network requests: https://github.com/tus/tus-js-client/blob/master/test/spec/upload.browser.js. If you want to, I can take care of the tests since it may take you too much time to understand them properly.

Let me know what you think about that :)

PS: The linter complains about a fix things but you should be able to take care of them by running eslint --fix

// the file blob, before uploading with tus.
if (isReactNative &&
typeof file === 'object' &&
file !== null) {

This comment has been minimized.

Copy link
@Acconut

Acconut Nov 17, 2018

Member

Could it be that you meant to write file.uri !== null?

This comment has been minimized.

Copy link
@arturi

arturi Nov 26, 2018

Author Contributor

Thanks!

arturi added some commits Nov 26, 2018

@arturi

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

Addressed 1 and 2!

Although in some places I’m not sure if variable should be called source, input or file. And then I’m saving reference to file in start() and _start().

On a more important note, integration tests are failing, I wonder if it’s because they don’t expect async getSource?

@Acconut

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

On a more important note, integration tests are failing, I wonder if it’s because they don’t expect async getSource?

It's not properly asynchronous even if we use a callback there since the callback is invoked synchronously. However, that's not the problem. The issue was that getSource didn't pass the result to the callback but instead returned the value.

I push a fix to your branch which solves this and also clean ups a few different problems that the new callback would introduce.

@Acconut

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

I also added two test cases since they were a bit harder to implement. Let me know what you think about them.

@arturi

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

Thanks for all the fixes, Marius! Silly me for some of them:

The issue was that getSource didn't pass the result to the callback but instead returned the value.

😱

Added “check for undefined instead of null”, because it was failing in React Native when actual blob was passed 😱. Not sure this is the 💯best way to make sure we are dealing with an object, which has a uri property, but seems ok?


req.responseError();

expect(options.onError).toHaveBeenCalledWith(new Error("tus: cannot fetch `file.uri` as Blob, make sure the uri is correct and accessible. [object Object]"));

This comment has been minimized.

Copy link
@arturi

arturi Dec 3, 2018

Author Contributor

[object Object]

I guess I need to remove the error text if we can’t display the actual message, [object Object] is not very useful. Maybe it can be converted to a string?..

This comment has been minimized.

Copy link
@Acconut

Acconut Dec 4, 2018

Member

Sounds good to me, would you be able to add that?

@Acconut

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

Not sure this is the best way to make sure we are dealing with an object, which has a uri property, but seems ok?

I think it's good enough for now. If a better way surfaces later, we can always patch it at another time :)

So, I think the only bigger thing is documentation, right?

Acconut added some commits Dec 8, 2018

Remove dependency on native base64 functions
React Native does not have the window.btoa function and were are
also not easily able to inject a polyfill only for React Native.
Therefore, we always use a non-native base64 implementation.
@Acconut

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

FYI, I added a small demo application for React Native and there was also some more work needed to get proper Base64 (used for upload metadata) support.

@Acconut

Acconut approved these changes Dec 9, 2018

@Acconut Acconut merged commit 921b8aa into tus:master Dec 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@arturi arturi deleted the arturi:feature/react-native branch Dec 10, 2018

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.