Skip to content

Enables parsing of application/hal+json as JSON#1853

Merged
martijnwalraven merged 6 commits into
apollographql:masterfrom
janhartmann:feature/parse-hal-as-json
Oct 26, 2018
Merged

Enables parsing of application/hal+json as JSON#1853
martijnwalraven merged 6 commits into
apollographql:masterfrom
janhartmann:feature/parse-hal-as-json

Conversation

@janhartmann

Copy link
Copy Markdown
Contributor

This pull requests enables JSON parsing of content type application/hal+json.

https://en.wikipedia.org/wiki/Hypertext_Application_Language

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@apollo-cla

Copy link
Copy Markdown

@janhartmann: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Oct 24, 2018
@janhartmann janhartmann changed the title Feature/parse hal as json Enables parsing of application/hal+json as JSON Oct 24, 2018
@martijnwalraven martijnwalraven merged commit 458bc71 into apollographql:master Oct 26, 2018
@martijnwalraven

Copy link
Copy Markdown
Contributor

Thanks, this looks good! I'm happy you also added a new test.

@jonbjk

jonbjk commented Nov 10, 2018

Copy link
Copy Markdown

I think the solution proposed in #1645
is more general, since it would allow parsing of any JSON response, by making the contentType configurable. In that way we can support for example "application/vnd.api+json" as well as "application/hal+json".

@janhartmann

janhartmann commented Nov 10, 2018

Copy link
Copy Markdown
Contributor Author

I agree, we also need to support different casing, e.g by lower casing.

As og today, if the content type sent back is “Application/Json” it will not parse either. Might need another issue

@jonbjk

jonbjk commented Nov 10, 2018

Copy link
Copy Markdown

In #1645 I added a field jsonContentType that defaults to "application/json", but that the developer can override (a bit like the baseUrl). I think that that is a simple and flexible solution.
If you look at for example: https://swagger.io/docs/specification/media-types/
a content type such as "application/vnd.mycompany.myapp.v2+json"
is also valid.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

⛲️ feature New addition or enhancement to existing solutions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants