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

feat(http): better binary support & XHR support #7707

Merged
merged 13 commits into from
Jan 28, 2020

Conversation

charsleysa
Copy link
Contributor

@charsleysa charsleysa commented Aug 21, 2019

PR Checklist

What is the current behavior?

HTTP module does not support uploading binary information, only text based information.
XHR doesn't follow spec.

What is the new behavior?

HTTP module now supports uploading binary information through the use of ArrayBuffer.
XHR now better follows spec, now has support for Blob and ArrayBuffer.

Implements #570.
Closes #7632.
Closes NativeScript/nativescript-angular#1202.

BREAKING CHANGES:

Possibly a breaking change if anyone is using the org.nativescript.widgets2.Async.Http Java class directly as RequestOptions.content has changed from a type String to type java.nio.ByteBuffer, though I'm not sure if this is considered to be an internal API.

Also, XHR is now more compliant with the spec which means some code that may have previously worked (or failed silently) will now cause an error.

Migration steps:
The content string must be converted to a java.nio.ByteBuffer instead of setting it directly to the request options. Here's an example of converting a content string:

        const contentString = '...blah...';
        const nativeString = new java.lang.String(contentString);
        const nativeBytes = nativeString.getBytes('UTF-8');
        const nativeBuffer = java.nio.ByteBuffer.wrap(nativeBytes);
        myhttpRequestOptions.content = nativeBuffer;

@cla-bot cla-bot bot added the cla: yes label Aug 21, 2019
@charsleysa charsleysa mentioned this pull request Aug 21, 2019
@rynop
Copy link
Contributor

rynop commented Aug 21, 2019

Thanks @charsleysa , really appreciate this. Few things:

  • How are you handling application/protobuf (binary) responses? AFAIK NS does not currently support (I think confirmed by the Reponse type of 'blob' not supported on HttpClient nativescript-angular#1202 you mention)
  • Do you have any examples/testcases? From looking at code should just need to set content to an ArrayBuffer however having something concrete to start from would save me time. Sounds like you use Angular - Angular ex would be perfect.

FWIW I'm willing to test and write the docs

@charsleysa
Copy link
Contributor Author

Hi @rynop

Regarding your first point, you are correct that binary download support is not currently available through angular due to XHR not having binary response support. I will add in arraybuffer responseType support to XHR and include it in this PR.

As for your second point, any angular tutorial for arraybuffer should work. Here's the http snippet from our app when we upload a user profile image:

                            this.http
                                .request(
                                    new HttpRequest('PUT', uploadUrl, data, {
                                        headers: new HttpHeaders({
                                            'Content-Type': fileType,
                                            'Content-Disposition': `attachment; filename="${fileName}"`
                                        })
                                    })
                                )

If you're able to write tests and docs that'd be a great help!

@charsleysa charsleysa changed the title feat(http): binary upload support feat(http): better binary support & XHR support Aug 22, 2019
@charsleysa
Copy link
Contributor Author

@rynop I've implemented full support (within limitations) for Blob and ArrayBuffer in both upload and download for XHR, updated the Fetch API polyfill. I've also added some tests but this PR will probably need a bit of testing against a few apps to make sure nothing is horribly broken.

tests/app/fetch/fetch-tests.ts Show resolved Hide resolved
tests/app/fetch/fetch-tests.ts Show resolved Hide resolved
tns-core-modules/http/http.ts Outdated Show resolved Hide resolved
@rynop
Copy link
Contributor

rynop commented Aug 28, 2019

@charsleysa I'm having trouble testing your fork. I'm following the NS dev guidelines without luck.

I'm:

  1. cloning your fork (I clone to ~/projects/clarsleysa-ns)
  2. git checkout feat-binary-upload
  3. npm install
  4. npm run compile-all
  5. From root of my NS proj: npm link ../clarsleysa-ns/tns-core-modules-widgets/ && npm link ../clarsleysa-ns/tns-core-modules
  6. tns debug ios --debug-brk --env.sourceMap

I get TONS of errors, here is one:

ERROR in node_modules/tns-core-modules/application/application-common.ts:36:5 - error TS2305: Module '"./application"' has no exported member 'AndroidApplication'.

When I look at the node_modules/tns-core-modules/http/http-request directory, I only see .ts files - I'm expecting to see the webpack'd/compiled .js files. I do see your .ts modifications however.

Any idea what I'm doing wrong?

I've tried rm -rf platforms and re-running tns debug ...

@charsleysa
Copy link
Contributor Author

@rynop did you solve your problem and get this running?
I have not tested linking the repo to another NS app project.

@rynop
Copy link
Contributor

rynop commented Aug 30, 2019

@charsleysa nope, I think there is a tns-core-modules bug in master (and therefore your fork/branch). Reproduction of bug and tracking of it is in "Update DevelopmentWorkflow.md for v6" link above.

@bundyo
Copy link
Contributor

bundyo commented Sep 20, 2019

The problem with linking is that Angular/TS project tend to grab the linked TS files in core modules and try to compile them without the references needed. I've only managed to run it by compiling the core modules with outDir and linking that (.d.ts, package.json and other files, except TS ones should be copied over too). Maybe the team can consider moving to an outDir compilation. :) Another way would be dev-webpack to forbid Angular/ts-loader for traversing node_modules.

@rynop
Copy link
Contributor

rynop commented Oct 3, 2019

@charsleysa It was a long (30+) day journey but I finally was able to verify the changes in the PR work great when using binary protobuf payloads. Can't tell you how much I appreciate this PR, I really think it is a big step forward for NS and gives a competitive advantage over React Native (which last time I checked has problems passing binary data over their JS bridge).

Now the details.

Workaround so I could test:
As suspected, the development workflow for contributing to tns-core-modules and tns-platform-declarations do not work when verifying using an Angular flavor NS app. After lots of investigation and debugging I have documented a workaround in #7861 (see files changed). It is nasty and the NS team agrees. They are working on a better approach.

What I tested
Binary support you've added for fetch/XHR is awesome. No longer limited to base64 encode or background thread which both have obv drawbacks. My main drive for binary support was to get protobuf working in NS.

In my test, I ran against a Twirp RPC server. For those curious, I used https://github.com/rynop/abp-sam-twirp almost verbatim.

I then created an Angular NS project, and an Angular service with the method below, which invokes the code in this PR.

  public createTwirpRPCAdapter(servicePathPrefix: string): $protobuf.RPCImpl {
    return (serviceMethodName: $protobuf.Method, requestData: Uint8Array, callback: Function) => {
      const req = new HttpRequest(
        'POST',
        servicePathPrefix + TwirpService.getServiceMethodName(serviceMethodName),
        // required to get an arraybuffer of the actual size, not the 8192 buffer pool that protobuf.js uses
        // see: https://github.com/dcodeIO/protobuf.js/issues/852
        requestData.slice().buffer,
        {
          headers: new HttpHeaders({
            'Accept': 'application/protobuf',
            'Content-Type': 'application/protobuf'
          }),
          responseType: 'arraybuffer',
        }
      );

      this.http.request<ArrayBuffer>(req)
        .subscribe(
          event => {
            console.log('event', event);
            if (event instanceof HttpResponse) {
              callback(null, new Uint8Array(event.body));
            }
          },
          (error: HttpErrorResponse) => {
            return callback(TwirpError.isTwirpError(error) ? new TwirpError(error) : error, null);
          }
        );
    };
  }

I successfully tested both green and error (500/404/400 etc) paths. I also verified binary in the request body AND binary in the response body.

There is quite a bit more to the protobuf aspect, but I don't want to bloat this PR. I'll blog and/or post more details for others wanting this use case in #7632

@bundyo the only issue I see is the potentially breaking change (org.nativescript.widgets2.Async.Http) mentioned above. Is this considered an internal API? If not do you know someone at Telerik / maintainer that can make the call? As stated above I think this PR has some significant value, so would hate to see it get held up too long.

@bundyo
Copy link
Contributor

bundyo commented Oct 4, 2019

@manoldonev Any chance this hitting 6.2?

@JoeyBG
Copy link

JoeyBG commented Oct 4, 2019

We really need this as well! Simply trying to upload a raw image to S3..

@manoldonev
Copy link
Contributor

@bundyo most probably will not ship with 6.2 but this PR is definitely on our radar and we will do our best to process it as soon as possible (and release it officially with 6.3)

@rynop
Copy link
Contributor

rynop commented Oct 10, 2019

I should have noted that my test above uses HttpRequest from @angular/common/http which under the covers uses XMLHttpRequest.

For completeness sake, I just tested POSTing protobuf via fetch like so:

  public createTwirpRPCAdapter(servicePathPrefix: string, headerOverrides?: HttpHeaders): $protobuf.RPCImpl {
    return (serviceMethodName: $protobuf.Method, requestData: Uint8Array, callback: Function) => {
      fetch(servicePathPrefix + TwirpService.getServiceMethodName(serviceMethodName),
        {
          method: 'POST',
          headers: {
            'Accept': 'application/protobuf',
            'Content-Type': 'application/protobuf'
          },
          body: requestData.slice().buffer,
        })
        .then(response => response.arrayBuffer())
        .then(body => callback(null, new Uint8Array(body)))
        .catch(error => {
          callback(TwirpError.isTwirpError(error) ? new TwirpError(error) : error, null);
        });
    };
  }

Sending binary in the request body and getting binary in the response body works in my tests via XMLHttpRequest and fetch.

@acivier-serial
Copy link

Hello!
Could you confirm that this feature will be present in the next release? (6.3)

@rynop
Copy link
Contributor

rynop commented Dec 20, 2019

@manoldonev since this did not make it into v6.3 as hoped, can you confirm it is slated to go into v6.4?

@vahidvdn
Copy link

vahidvdn commented Jan 2, 2020

Is the PR stuck for some reason?

@rynop
Copy link
Contributor

rynop commented Jan 2, 2020

From my perspective it is ready to go. I tested multiple scenarios on multiple devices/emulators/simulators. I'm guessing the team wants to get more testing (wise).

@vtrifonov vtrifonov requested a review from vmutafov January 27, 2020 15:47
@vtrifonov vtrifonov merged commit e293367 into NativeScript:master Jan 28, 2020
@rynop
Copy link
Contributor

rynop commented Jan 28, 2020

Wahooo!!! Thanks for everyone's hard work on this! Especially you @charsleysa

@vibos
Copy link

vibos commented Jul 1, 2020

I'm trying to upload image in binary mode using Angular's HttpClient. I tried different options, also tried using fetch and xhr, but nothing works.
I have ImageSource as input, then saving it to filesystem and need to send POST request with multipart/form-data type.
Could anyone please provide code snippet how to do that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protobuf support Reponse type of 'blob' not supported on HttpClient