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 JSON responses #250

Merged
merged 3 commits into from Mar 1, 2021
Merged

Add JSON responses #250

merged 3 commits into from Mar 1, 2021

Conversation

tilgovi
Copy link
Contributor

@tilgovi tilgovi commented Jan 28, 2021

Description of Change

Return HTML or JSON , with the format chosen to match the request accept
header.

Additionally, rewrite requests with URLs that end in ".json" to their
equivalent document without an extensnion but with the appropriate
accept header to receive a JSON response.

Motivation and Context

I'd like to embed content from a Library instance in another product. Library is under active use and a different group of people manages its content than the team responsible for the product into which the Library content will be embedded. For maintainability, it's preferable to fetch that content from Library on the fly, and not be subject to the templating in Library.

Checklist

  • Ran npm run lint and updated code style accordingly
  • npm run test passes
  • PR has a description and all contributors/stakeholder are noted/cc'ed
  • tests are updated and/or added to cover new code
  • relevant documentation is changed and/or added

Return HTML or JSON , with the format chosen to match the request accept
header.

Additionally, rewrite requests with URLs that end in ".json" to their
equivalent document without an extensnion but with the appropriate
accept header to receive a JSON response.
@tilgovi
Copy link
Contributor Author

tilgovi commented Jan 28, 2021

I've left the last two bullet points un-checked. I'm seeking feedback on:

  • Am I exposing the right fields, with the right names? What do we want this API to expose?
  • Once we settle the previous question, maybe I should factor some helpers out. Can you suggest anything?
  • I want to add additional test coverage for this, but want to resolve these questions first.
  • Any documentation that seems relevant to add?

@midorikocak
Copy link

Following

@isaacwhite
Copy link
Collaborator

@tilgovi Thanks so much for taking on this feature, which I think will be useful for lots of programmatic use cases.

What you have so far seems right to me, the only thing I've spotted is that it looks like the tests are not passing because the deep equal assertion is finding additional fields in the test data that are not part of the match. I wonder if using objectContaining instead of deepEqual would solve that, and then we could move forward with finalizing and merge?

I also see you are adding JSON support for the search endpoint, which could be a help for another use case I heard about recently. (cc @crystalnyt)

@tilgovi
Copy link
Contributor Author

tilgovi commented Feb 18, 2021

Thanks, @isaacwhite. I just wanted some confirmation that the fields being returned look correct and like they're named in a sane and consistent way. I'll get the tests passing and come back today/tomorrow.

@tilgovi tilgovi marked this pull request as ready for review February 25, 2021 17:08
@midorikocak
Copy link

Still following.

server/routes/pages.js Outdated Show resolved Hide resolved
Co-authored-by: Isaac White <isaacwhite@users.noreply.github.com>
Copy link
Collaborator

@isaacwhite isaacwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏🏻 This is great, thanks so much for building support for this into Library core @tilgovi and @smores! We're really excited by the integrations that it will make possible with other services, and plan to take advantage of it for some future issues (like #242), which could integrate well with these new endpoints.

Let me know if you have any other questions. I plan to merge this later today.

@isaacwhite isaacwhite merged commit b7b068a into nytimes:master Mar 1, 2021
@tilgovi tilgovi deleted the json-responses branch March 8, 2021 18:48
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.

None yet

4 participants