-
Notifications
You must be signed in to change notification settings - Fork 338
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
Support Wrappers in Nestjs #852
Comments
For reference, bellow my buf config : ...
plugins:
- plugin: buf.build/community/stephenh-ts-proto
out: ./packages/typescript/gen
opt:
- nestJs=true
- useDate=true |
Still looking to the lib, trying to figure it out and one question came to my mind: Is it possible that the fix for wrapper is actually available, but the version of protobuf used in my nest application being not the same than the one based on ts-proto, the patch is not applied?
|
Re-aligned the version and it does not change a thing 😭
|
Tried the workaround mentioned here using :
but the code is never called.
|
Hi @julien-sarazin , apologies for the delay, I only have the opportunity to triage ts-proto issues every few weeks or so. I think you are right, we don't create a wrapper for StringValue atm...I think it would be pretty easy, just a matter of copy/pasting this timestamp-based one: https://github.com/stephenh/ts-proto/blob/main/src/main.ts#L423 Or, while prototyping, you could just write a StringValue-version of this directly in the If you wanted to submit a PR for this, that'd be great, and you could probably add a StringValue field to this existing nestjs integration test: https://github.com/stephenh/ts-proto/blob/main/integration/nestjs-simple/hero.proto The readme has an overview of contributing, but lmk if you have questions. Thanks! |
Hi @stephenh thanks for your reply. I reached a point digging into // type.js
Type.prototype.setup = function setup() {
// Sets up everything at once so that the prototype chain does not have to be re-evaluated
// multiple times (V8, soft-deopt prototype-check).
var fullName = this.fullName,
types = [];
for (var i = 0; i < /* initializes */ this.fieldsArray.length; ++i)
types.push(this._fieldsArray[i].resolve().resolvedType);
// Replace setup methods with type-specific generated functions
this.encode = encoder(this)({
Writer : Writer,
types : types,
util : util
});
this.decode = decoder(this)({
Reader : Reader,
types : types,
util : util
});
this.verify = verifier(this)({
types : types,
util : util
});
this.fromObject = converter.fromObject(this)({
types : types,
util : util
});
this.toObject = converter.toObject(this)({
types : types,
util : util
});
// Inject custom wrappers for common types
var wrapper = wrappers[fullName];
if (wrapper) {
var originalThis = Object.create(this);
// if (wrapper.fromObject) {
originalThis.fromObject = this.fromObject;
this.fromObject = wrapper.fromObject.bind(originalThis);
// }
// if (wrapper.toObject) {
originalThis.toObject = this.toObject;
this.toObject = wrapper.toObject.bind(originalThis);
// }
}
return this;
}; Doing what you suggested will add the Thanks in advance for any help on that matter. |
Huh! That is really odd... I set a breakpoint in the current
Which is in a generated file; honestly I did not realize that protobufjs was doing run-time generation of serialization code...oh, actually that makes sense, b/c NestJS is going through grpc-js which is going through protoloader, so it's the protoloader that assumes we're using stock protobufjs messages. That makes sense, but yes, still really odd wrt StringValue's Do you mind pushing up a PR of what you've got so far? If I have time I'll set a few break points here & there and maybe see if I can guess what's going on. I did some searches for "Timestamp" in the protobufjs codebase and didn't see anything sticking out, like a LOC that says "make Struct & Timestamp look up in wrappers but not anything else". 🤔 Thanks! |
MR is on its way 👍
While cleaning my mess up, I was thinking that Google wrappers do not have their own proto files; they are all imported under Sorry if I am really out of it, I'm new to protobuf ecosystem, protoc, and not used to code generation libraries! |
Hello @stephenh, any chance to have a look this week? |
Friendly 🆙 |
Hi @julien-sarazin ; finally got a chance to take a look at this. Thank you for putting together this repro/initial PR! With this example, it looks like this issue is that NestJS goes through grpc.js, which goes through proto-loader, which goes through protobufjs, and specifically protobufjs's So the code ends up looking like:
And the issue is that we want It turns out this is a known issue in protobufjs: protobufjs/protobuf.js#677 (comment) And even has a super-short PR open to fix it: Unfortunately the PR has not been merged in the ~4 years since it's been open. I just pinged the maintainers to see if they'll merge it 🤞 : protobufjs/protobuf.js#1271 (comment) But currently I think we're blocked on that. I did a little bit of exploration to see if we could side-step their runtime-generated converter/encoders, and just use the ones that ts-proto generates itself, but I'm not finding a way to slice that into proto-loader before it hits the problematic |
Hey @stephenh, I just wanted to give you a big thank you for looking into this! I'm really glad to know what needs to be fixed, although it's a bit disappointing that we might not see it resolved for a while. Your time and effort on this are truly appreciated, and I want to give you a shoutout for pinging the guys from protobufjs about the PR! 🙌 In the meantime, we'll make do with the |
Hey! Thanks for the amazing lib 👍 I'm trying to integrate it with NestJS, which works very well for 99% of my use cases, the remaining 1% is the support of Google Wrappers.
Defining the following proto file :
Then during the object's serialization when the gRPC endpoint is called give the following error :
Seems to be related to
I am not yet sure to clearly understand the ts-proto, protobuf.js, @grpc/proto-loader's responsibilities. From what I understood :
ts-proto
is in charge of generating the TS/JS representation of Proto files that will be used by protobuf.jsprotobuf.js
is the actual engine that serializes/deserializes using the ts-proto definitions@grpc/proto-loader
uses protobuf.js in its "server" when gRPC calls are made.Now focusing on my issue, I understood that when you put the
nestJs
option you're supposed to have customized wrappers code generated (like the example you have here).The first issue is, (I am using buf) when I generate TS definition I don't have this custom override.
The second is, I don't see any support for StringValue.
Could you help me on that? And if needed I would be glad to add the support for more wrappers with a little guidance 😅
Thanks!
The text was updated successfully, but these errors were encountered: