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

Change JsonContent to return object rather than string #379

Merged
merged 4 commits into from
May 18, 2022

Conversation

egmacke
Copy link
Contributor

@egmacke egmacke commented May 6, 2022

Description

Resolves issue #378 by allowing objects to be sent without converting to strings.

Related Issue

Issue #378

Motivation and Context

Solves the problem where some middleware expects Json objects rather than strings. Example: openapi-validator-middleware

How Has This Been Tested?

Updated unit tests.

Functionally tested by spinning up a server with various endpoints that return both Json and non-json responses.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

This change could possibly be a breaking change in downstream implementations where middleware has been configured to expect JsonResponse

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@PodaruDragos
Copy link
Member

overall I am ok with this PR, but please find a better type for object like Record<string, unknown>

@egmacke
Copy link
Contributor Author

egmacke commented May 16, 2022

@PodaruDragos thanks for the feedback. I've updated as suggested. I'd originally used object to avoid being overly opinionated, but I agree that using Record is a better solution.

@PodaruDragos
Copy link
Member

LGTM, thanks for this

@PodaruDragos PodaruDragos merged commit ba24008 into inversify:master May 18, 2022
@egmacke
Copy link
Contributor Author

egmacke commented May 18, 2022

@PodaruDragos thanks.
@dcavanagh What's the release cycle for this repo? Just wondering when I'll be able to make use of this as part of a released version?

@Jameskmonger
Copy link
Member

@egmacke sorry for the delay, I will publish this as 6.4.4 when I finish work today (#390)

@Jameskmonger
Copy link
Member

Pleased to say this is now published on npm in version 6.4.4 @egmacke

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.

3 participants