feat(nest): Change the nest integration to support Nest features that were competing with Orpc #1149
Conversation
|
@Mathiasduc is attempting to deploy a commit to the unnoq-team Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @unnoq This is the PR we were talking about in #992 . |
…at were competing with Orpc
Mathiasduc
force-pushed
the
feat/server/nest/add-support-for-nestjs-features
branch
from
6892585 to
8366106
Compare
There was a problem hiding this comment.
I still don’t understand why we need to use return just to increase compatibility with NestJS. Modifying the response body is an anti-pattern and makes it incompatible with the generated openapi spec. Why not modify the headers instead? We can already change headers without requiring a return.
|
|
||
| const client = createProcedureClient(procedure, { | ||
| ...this.config, | ||
| context: contextWithRequest, |
There was a problem hiding this comment.
This approach is not typesafe. While implement expects an empty context {}, this can lead to problems. For instance, a middleware might expect a context like { request?: number | undefined }. Because an empty object {} doesn't conflict can satisfy middleware's dependent-context so we can .use when implement. But in this case request is req -> middleware can run failed because middleware expect request is undefined or number
| * @param headers - WARNING: The headers can be mutated by the function and may affect the original headers. | ||
| * @param options | ||
| */ | ||
| export function toResponseBody( |
There was a problem hiding this comment.
I don't think this package needs these APIs. I believe we should write it inside the nest package instead. We can implement some of them as signal functions
There was a problem hiding this comment.
I'll try to not need to define this function since it's so similar to toNodeHttpBody. Adding an option to stringify the body or not should be enough.
We can implement some of them as signal functions
I did not understand what you meant by that.
There was a problem hiding this comment.
I mean you can write a signal function in nest package instead.
| import * as StandardServerFastify from '@orpc/standard-server-fastify' | ||
| import * as StandardServerNode from '@orpc/standard-server-node' | ||
|
|
||
| const codec = new StandardOpenAPICodec( |
There was a problem hiding this comment.
I'm looking for a way to make all codecs extendable. Specifically, I'd like to know if it's possible to add a custom serializer by passing it into the main configuration, similar to the method described for extending native data types.
| // Set status and headers | ||
| if ('raw' in res) { | ||
| await StandardServerFastify.sendStandardResponse(res, standardResponse, this.config) | ||
| return StandardServerFastify.setStandardResponse(res as FastifyReply, standardResponse, this.config) |
There was a problem hiding this comment.
Make sure you tests there cases:
undefinedin this case expect body is empty + no content-type is setjson datain this case expect returnjson data(not stringify) + content-type is jsonfile/blobin this case expect streaming response + content-type = file.type
Mathiasduc
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
More templates
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/experimental-publisher
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fastify
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
Not from inside a Nest/Express/Fastify middleware if the response has already been sent by Orpc
It is but, I feel like, this should not be a concern of this integration. There is legitimate reasons to modifying a response defined by Orpc and ways to do it so it will not break contract/specs.
This features exist in Orpc eco-system, with plugin or external packages, and users can also implement them at the ORPC level (orpc's middleware/interceptors) but why not make it possible to delegate this features to the platform being integrated with, Nest in this case. But, indeed, it also let the door open to the users to shoot themselves in the foot and break their own contract and specs. |
|
We can move this discussion in an issue if you prefer |
Reasonable, and I agreed - I just don't want mess thing up by introducing new apis, if you do this good enough I have no reason to reject.
I believe without |
After thinking about it more, can you provide real examples or popular npm packages that depend on this? Personally, I believe that under the hood they should be Express middleware or Fastify plugins - so I don't think they need Also, regarding your implementation, we can't always |
| } | ||
| }) | ||
|
|
||
| resBody.once('error', error => res.destroy(error)) |
There was a problem hiding this comment.
In Node.js we manually send streaming responses, but in Fastify you return the stream - this creates inconsistent behavior.
2 participants
This PR all the tested features of Nest to integrate better with orpc and it's feature.
For example:
ORPCExceptionFilterto keep the same behaviour.Basically everything that was trying to modify the response after the handler was failing due to the handler sending the response.
The handler now set headers, status code, return the proper body and let the middleware chain resolve and let Nest/Node send the response at the end.