strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Support application/vnd.api+json media type #249

Closed digitalsadhu closed 8 years ago

digitalsadhu commented 8 years ago

Adds support for application/vnd.api+json media type.

See discussion here: https://github.com/strongloop/loopback/issues/445#issuecomment-150923129

listepo commented 8 years ago

:+1:

digitalsadhu commented 8 years ago

@bajtos @ritch any chance you guys can have a look at this? Happy to make chances as necessary.

ritch commented 8 years ago

@digitalsadhu this looks good to me. Thanks for the patch :smile: :+1:

ritch commented 8 years ago

@slnode test please

ritch commented 8 years ago

I think we should mention this in the docs somewhere... Any ideas @digitalsadhu ? (this is not a blocker for merging this BTW).

digitalsadhu commented 8 years ago

Ok. Will have a scan of the docs and report back.

digitalsadhu commented 8 years ago

@ritch 1 thing I've wondered is whether I should have added application/vnd.api+json to the default supported types or not? If not then we should definitely document that the user will need to do this (most likely in config.json I guess) if they want to use application/vnd.api+json in the accept header and have it recognized. Thoughts?

digitalsadhu commented 8 years ago

On that note, adding documentation here https://docs.strongloop.com/display/public/LB/config.json#config.json-Remotingproperties Would be one place

ritch commented 8 years ago

Here are the default types: https://github.com/strongloop/strong-remoting/pull/249/files#diff-c8e4ad547ed92deb50d4eb3bc6430335R18

The issue I see adding as a default is, when a client says they accept this type of JSON, we aren't using that information to give them what they are asking for. So for now we should just add a note to the docs saying that you can opt into the support (eg. what you did in your test).

@crandmck can you make this change ^^^ ?

digitalsadhu commented 8 years ago

Yup. Sounds good to me.

crandmck commented 8 years ago

Re docs

can you make this change ^^^ ?

Sure, but I will wait for the PR to be merged, which I assume s/be soon.

crandmck commented 8 years ago

Went ahead and added to docs, since I presume the merge will happen soon. Thx for the heads up!

digitalsadhu commented 8 years ago

@ritch Whats the process for merging now? Need me to do anything further?

ritch commented 8 years ago

@digitalsadhu sorry for delayed responses :( - pinging me via a comment is the best way to bring something like this to the top of my queue. Thanks again for the contribution.

digitalsadhu commented 8 years ago

@ritch @crandmck Just noticed the doc update here under supportedTypes: https://docs.strongloop.com/display/public/LB/config.json

I think it isn't quite correct. We decided not to add application/vnd.api+json to the default set of supported types so it needs to be added manually. The docs have it under "default". It's more that it's one of the possible options you can set but not there by default.

crandmck commented 8 years ago

@digitalsadhu Thanks for clarifying! I updated the doc.
Can you tell me how one adds it as a supportedType? That would probably be useful to note as well.

digitalsadhu commented 8 years ago

@crandmck great!

You can add it as a supportedType either in config.json as such:

{
  "remoting": {
    "rest": {
      "supportedTypes": ["application/vnd.api+json", "application/json", ...]
    }
  }
}

or programatically via something like:

app.remotes().options.rest.supportedTypes =  ['application/vnd.api+json', 'application/json', ...]

I think, if at all possible, it would be good to document somewhere that setting the Accept header on the request to application/vnd.api+json will result in the Content-Type header being automatically set to application/vnd.api+json on the response so long as application/vnd.api+json is in the array of supported types.

crandmck commented 8 years ago

Thanks--I added that last part to https://docs.strongloop.com/display/LB/Exposing+models+over+REST#ExposingmodelsoverREST-Requestformat.

digitalsadhu commented 8 years ago

Nice, awesome!