Fix grpc channel options #8365
Fix grpc channel options #8365
Conversation
Sync with master
…r read in the code). Therefore, declaring those ChannelOptions had no effect anymore and there was no way to configure specific gRPC settings & limits. Also extended ChannelOptions with grpc-node.max_session_memory which is required when sending big messages (see grpc/grpc-node#1666). Ensured backward compatibility by including size limits like grpc.max_send_message_length to the ChannelOptions
Removed GRPC_DEFAULT_MAX_RECEIVE_MESSAGE_LENGTH and GRPC_DEFAULT_MAX_SEND_MESSAGE_LENGTH constants. This default values are already the same in the underlying gRPC library. If the gRPC team decides to change that values in the future, nestjs should adopt the new values automatically.
| @@ -14,6 +12,7 @@ import { InvalidProtoDefinitionException } from '../errors/invalid-proto-definit | |||
| import { ClientGrpc, GrpcOptions } from '../interfaces'; | |||
| import { ClientProxy } from './client-proxy'; | |||
| import { GRPC_CANCELLED } from './constants'; | |||
| import {ChannelOptions} from "../external/grpc-options.interface"; | |||
There was a problem hiding this comment.
Please run prettier npm run format.
| export const GRPC_DEFAULT_MAX_RECEIVE_MESSAGE_LENGTH = 4 * 1024 * 1024; | ||
| export const GRPC_DEFAULT_MAX_SEND_MESSAGE_LENGTH = 4 * 1024 * 1024; | ||
|
|
There was a problem hiding this comment.
I think it is worth returning these default values and using them when the developer didn't provide them explicitly in the options.
There was a problem hiding this comment.
4 MB is the current gRPC default, therefore nothing changes for developers who did not set a value. Why redeclare? And if the gRPC default changes (for reasons) in the futurore, why would nestJS want to stick to outdated default values?
There was a problem hiding this comment.
Gotcha then yep it makes sense to remove these options.
|
Also, commit messages don't follow project conventions, please read the guideline https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md |
|
How crucial ist the commit message format for you guys? It's a bit nasty to change them afterwards. However, this issue made us downgrade to nestjs 7, and I believe anyone else who uses nestjs with gRPC in production needs to do so as well. My problem with the current situation is:
Can you give some advice how that dependency can be decoupled? |
Not a big deal, feel free to ignore it. I'll look into this PR as soon as I can. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current vs. new behavior?
The GrpcOptions attribute "channelOptions" was not used anymore (never read in the code). Therefore, declaring those ChannelOptions had no effect anymore and there was no way to configure specific gRPC settings & limits.
I fixed that and also extended ChannelOptions with grpc-node.max_session_memory which is required when sending big messages (see grpc/grpc-node#1666).
Ensured backward compatibility by including size limits like grpc.max_send_message_length to the ChannelOptions
Does this PR introduce a breaking change?