Change client to return instances of our classes. #3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This changes the client code generated for `--target thrift-server` so that it returns instances of the generated thrift classes instead of plain objects. I believe the client code generated for `--target apache` already behaves this way. One advantage is this allows `instanceof` to be used on Thrift responses, which makes exceptions [distinguishable from each other](creditkarma#181) using `instanceof`. It would also increase the usefulness of having [exception classes derive from Error](creditkarma#178). The change itself is small and easy. The compiler already generates "MyEndpoint__Result" classes that have a read() function that instantiates the class, I just needed to call it from the appropriate place in the Client class. To review this change it'll be helpful if you're familiar with the various classes that are generated. I'll refrain from trying to describe it here--you're better off looking some generated code on your own. Here's a diff showing how this change affects a generated ping function. This is in the Client class: ``` public ping(context?: Context): Promise<string> { const writer: thrift.TTransport = new this.transport(); const output: thrift.TProtocol = new this.protocol(writer); output.writeMessageBegin("ping", thrift.MessageType.CALL, this.incrementRequestId()); const args: IPing__ArgsArgs = {}; Ping__ArgsCodec.encode(args, output); output.writeMessageEnd(); return this.connection.send(writer.flush(), context).then((data: Buffer) => { const reader: thrift.TTransport = this.transport.receiver(data); const input: thrift.TProtocol = new this.protocol(reader); try { const { fieldName: fieldName, messageType: messageType }: thrift.IThriftMessage = input.readMessageBegin(); if (fieldName === "ping") { if (messageType === thrift.MessageType.EXCEPTION) { const err: thrift.TApplicationException = thrift.TApplicationExceptionCodec.decode(input); input.readMessageEnd(); return Promise.reject(err); } else { - const result: IPing__Result = Ping__ResultCodec.decode(input); + const result: Ping__Result = Ping__Result.read(input); input.readMessageEnd(); if (result.success != null) { return Promise.resolve(result.success); } else { return Promise.reject(new thrift.TApplicationException(thrift.TApplicationExceptionType.UNKNOWN, "ping failed: unknown result")); } } } else { return Promise.reject(new thrift.TApplicationException(thrift.TApplicationExceptionType.WRONG_METHOD_NAME, "Received a response to an unknown RPC function: " + fieldName)); } } catch (err) { return Promise.reject(err); } }); } ``` You'll notice that I also changed the type of `result` from `IPing__Result` to `Ping__Result`. This isn't important and could be reverted, but I think it's better this way. I like keeping types specific until the point where it's useful for them to be generic. I'd advocate for making the same change to the types of struct class members (would need to change these types, I think: https://github.com/creditkarma/thrift-typescript/blob/e8931ff0d98c871360af99bbfa805cfd088d2f0e/src/main/render/thrift-server/struct/class.ts#L293 and https://github.com/creditkarma/thrift-typescript/blob/e8931ff0d98c871360af99bbfa805cfd088d2f0e/src/main/render/thrift-server/struct/reader.ts#L51), though that isn't of immediately use to me personally and I don't intend to pursue it. Remaining work: - Fix tests.
BuckAMayzing
approved these changes
Aug 31, 2020
p3l6
approved these changes
Sep 1, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm merging this change into the all_states_title_changes branch. For the full PR description see the PR for merging this change into Credit Karma's master branch.