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

Consider removing UJSONResponse from core #901

Closed
lithammer opened this issue Apr 15, 2020 · 2 comments · Fixed by #1047
Closed

Consider removing UJSONResponse from core #901

lithammer opened this issue Apr 15, 2020 · 2 comments · Fixed by #1047
Labels
clean up Refinement to existing functionality

Comments

@lithammer
Copy link

lithammer commented Apr 15, 2020

Sorry if this isn’t the place to discuss this. I created this issue for visibility of a comment I made in a pull request to add a ORJSONResponse class (similar to the UJSONResponse class). I'm gonna quote it here:

[...] I don't think flavour of the month JSON libraries belong in Starlette core, this applies to UJSONResponse as well. Next up someone wants a SIMDJSONResponse class[1]. Where should the line be drawn?

Another aspect to consider is the maintenance burden of implementing custom response classes like this in core, because that means a contract has been signed. So if orjson becomes unmaintained tomorrow, Starlette would have to go through a deprecation cycle to remove it. Or keep it forever... Bugs in the renderer(s) would likely end up as a Starlette issue as well, instead of in the upstream JSON library.

But enough complaining and on to more constructive feedback. It's evident by this pull request that it's pretty trivial to implement your own custom JSONResponse class, so maybe just a section in the documentation describing how to do it is enough? Then someone can (and probably will) publish a package to PyPI that Starlette later can link to, e.g. starlette-orjson. Maybe even a starlette-contrib repo under the encode org? 🤷‍♂️

Another option is to make the default JSONResponse more pluggable/configurable, similar to how you can configure renderers in Django REST Framework[2]. Personally, I like this approach.

[...]

[1] https://github.com/TkTech/pysimdjson
[2] https://www.django-rest-framework.org/api-guide/settings/#default_parser_classes

@lithammer
Copy link
Author

It seems like this has already been hinted at by @tomchristie in a another issue as well, see #304 (comment).

@lithammer lithammer changed the title Remove UJSONResponse from core Consider removing UJSONResponse from core Apr 15, 2020
@tomchristie
Copy link
Member

Yup, indeed - I'm in favour of removing it from core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean up Refinement to existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants