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 "file" return args #284

Merged
merged 2 commits into from
Feb 26, 2016
Merged

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Feb 24, 2016

  1. Fix handling of return args with http.target:header. Arguments mapped to a response http header should not be included in the response body.

  2. Allow users to specify { type: 'file', root: true } for the single argument that will be sent directly as the response body.

The following values are supported for file the file argument:

  • String
  • Buffer
  • ReadableStream (anything that exposes .pipe() method)

Example usage:

MyModel.download = function(cb) {
  // this can be for example http.request()
  getTheStreamBody(function(err, stream) {
    if (err) return cb(err);
    // stream can be any of: string, buffer, ReadableStream (e.g. http.IncomingMessage)
    cb(null, stream, 'application/octet-stream');
  });
}

MyModel.remoteMethod('download', {
  isStatic: true,
  returns: [
    { arg: 'body', type: 'file', root: true },
    { arg: 'Content-Type', type: 'string', http: { target: 'header' } },
  ],
});

Fix strongloop/loopback#2063
Connect to strongloop/loopback#213
Connect to #35

/to @ritch please review
/cc @raymondfeng @STRML

"returns" arguments mapped to an http header should not be included
in the response body.
@bajtos bajtos force-pushed the feature/stream-buffer-returns branch from 3953bfd to 7f1cef1 Compare February 25, 2016 08:10
@bajtos
Copy link
Member Author

bajtos commented Feb 25, 2016

I have simplified and reworked this feature. Instead of adding a new flag, I added a new type called "file":

{
  returns: { arg: 'body', type: 'file', root: true }
}

This way the feature is focused on developer's intention (I want to return a raw file response) instead of implementation details (I want to skip json/xml encoding and send the value in raw form). This should make it easier for consumers of remoting metadata (e.g. loopback-swagger) to correctly handle this case.

@ritch
Copy link
Member

ritch commented Feb 25, 2016

We should add a note to these docs: http://apidocs.strongloop.com/strong-remoting/#sharedmethod about the new file type.

Otherwise LGTM 👍

@ritch ritch assigned bajtos and unassigned ritch Feb 25, 2016
@bajtos
Copy link
Member Author

bajtos commented Feb 26, 2016

We should add a note to these docs: http://apidocs.strongloop.com/strong-remoting/#sharedmethod about the new file type.

Added, see 0d4ea21

I'd like to document this feature in more details as part of #35, I'll probably create a new "doc" story for that.

Allow users to specify { type: 'file', root: true } for the single
argument that will be sent directly as the response body.

The following values are supported for file the file argument:
 - String
 - Buffer
 - ReadableStream (anything that exposes .pipe() method)
@bajtos bajtos force-pushed the feature/stream-buffer-returns branch from 0d4ea21 to a85068e Compare February 26, 2016 09:25
@bajtos bajtos changed the title Stream and Buffer returns Stream and Buffer return arg Feb 26, 2016
@bajtos bajtos changed the title Stream and Buffer return arg Add new return arg type "file" Feb 26, 2016
@bajtos bajtos changed the title Add new return arg type "file" Add support for "file" return args Feb 26, 2016
bajtos added a commit that referenced this pull request Feb 26, 2016
@bajtos bajtos merged commit a6a75fc into master Feb 26, 2016
@bajtos bajtos deleted the feature/stream-buffer-returns branch February 26, 2016 09:54
@bajtos bajtos removed the #review label Feb 26, 2016
bajtos added a commit that referenced this pull request Feb 26, 2016
…-2.x

Add support for "file" return args [2.x]

A back-port of #284
@raymondfeng
Copy link
Member

It's a great feature. What about accepts? If one of the args is file, we should only handle the content-type multipart/form-data. See http://swagger.io/specification/

@bajtos
Copy link
Member Author

bajtos commented Apr 7, 2016

It's a great feature. What about accepts? If one of the args is file, we should only handle the content-type multipart/form-data. See http://swagger.io/specification/

@raymondfeng sounds good, please open a new issue to keep track of this feature request.

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

Successfully merging this pull request may close these issues.

4 participants