bleupen / halacious

a better HAL processor for Hapi
MIT License
108 stars 22 forks source link

HALify only when negotiating a HAL mediaType #77

Closed antoine-richard closed 8 years ago

antoine-richard commented 9 years ago

Hi. With this PR, I would like to discuss how to best handle the issue I have

halify only when negotiating a HAL mediaType (but not another mediaType handled by the app)

Here's my use-case: I have an app which handles hal+json and json. When returning json, the representation isn't wrapped with HAL. All is fine here.

My problem is that the app returns a hal+json representation by default (i.e with no accept header,which is equivalent to accept: */*). I'd have preferred json :/

Have I missed something?

If not, I tried to solve this issue with this PR in which:

Best, Antoine

bleupen commented 9 years ago

Hello and thanks for the PR!

Would you mind working up a couple of unit tests to spell out the use cases you have in mind?

antoine-richard commented 9 years ago

Sure! I'll get back to you as soon as possible.

antoine-richard commented 8 years ago

Hi,

I've pushed a test illustrating my use-case. I took the opportunity to slightly rework the API. Hopefully it will be more understandable.

Please let me know what you think.

bleupen commented 8 years ago

Awesome, thank you!

Sent from my iPhone

On Nov 29, 2015, at 4:52 PM, Antoine Richard notifications@github.com wrote:

Hi,

I've pushed a test illustrating my use-case. I took the opportunity to slightly rework the API. Hopefully it will be more understandable.

Please let me know what you think.

— Reply to this email directly or view it on GitHub.

antoine-richard commented 8 years ago

Hi, How can I help you to get this merged?

bleupen commented 8 years ago

Hi Antoine,

Thanks for the nudge. My family and I moved houses over the holidays so my mind has been elsewhere.

I'll take a look at it this afternoon. Shouldn't take too long.

Brad

Sent from my iPhone

On Dec 30, 2015, at 1:20 PM, Antoine Richard notifications@github.com wrote:

Hi, How can I help you to get this merged?

— Reply to this email directly or view it on GitHub.

bleupen commented 8 years ago

Ok just looked through the code. I simplified the configuration a little by adding a requireHalJsonAcceptHeader boolean configuration parameter, which triggers hal processing only if the client explicitly requests application/hal+json Sound good?

antoine-richard commented 8 years ago

Hi Brad,

Sorry, didn't want to bother you during holidays. Hope the move went well.

This simplification looks good to me. I just want to make sure the following cases are supported:

Let's say the client request no particular content-type (i.e. */*) and:

  1. the server is configured to return application/hal+json by default? (not implemented in the tests I pushed)
  2. the server is configured to return another media-type (application/json for example) by default? (tested)

Can I see your code somewhere?

bleupen commented 8 years ago

ok back from holidays. i pushed my code up to a "conneg" branch

antoine-richard commented 8 years ago

Hi,

Tested the conneg branch, thank you. It works for me :+1: