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 ORJSONResponse #870

Closed
wants to merge 1 commit into from
Closed

Conversation

hidaris
Copy link

@hidaris hidaris commented Mar 20, 2020

Add ORJSONResponse, including docs and tests

@lithammer
Copy link

lithammer commented Apr 15, 2020

Sorry for the drive-by comment. Maybe it's just me, but 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.

Obviously this is just my random opinion in the matter, so feel free to ignore it 🙂

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

@erewok
Copy link
Contributor

erewok commented Apr 15, 2020

I agree with @lithammer on this completely. We use RapidJSON on our projects, but I have no desire to see it directly incorporated into Starlette.

@lithammer
Copy link

FYI I created an issue related to this after posting my comment: #901

@abersheeran
Copy link
Member

I agree with @lithammer @erewok on this completely.

By the way, maybe UJSONResponse should also be removed.

It is up to the users to decide whether to use a high-performance JSON parser. All we need to do is give them a good interface for easy customization.

@tomchristie tomchristie closed this May 6, 2020
@lithammer
Copy link

By the way, maybe UJSONResponse should also be removed.

See #901.

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.

5 participants