Don't output variables that are not used in typescript service definition #38
Conversation
|
@jonnyreeves added a test but not sure if there is a better way to test this than checking the generated source code for the import. Let me know what you think. |
src/ts/fileDescriptorTSServices.ts
Outdated
| return service.getMethodList().some(method => { | ||
| const requestMessageTypeName = getFieldType(MESSAGE_TYPE, method.getInputType().slice(1), "", exportMap); | ||
| const responseMessageTypeName = getFieldType(MESSAGE_TYPE, method.getOutputType().slice(1), "", exportMap); | ||
| if (requestMessageTypeName.indexOf(pseudoNamespace + ".") === 0) { |
There was a problem hiding this comment.
These checks can be collapsed, ie:
const namespacePackage = pseudoNamespace + ".";
return requestMessageTypeName.indexOf(namespacePackage) === 0 || responseMessageTypeName.indexOf(namespacePackage) === 0
src/ts/fileDescriptorTSServices.ts
Outdated
| @@ -25,6 +41,10 @@ export function printFileDescriptorTSServices(fileDescriptor: FileDescriptorProt | |||
|
|
|||
| fileDescriptor.getDependencyList().forEach((dependency: string) => { | |||
| const pseudoNamespace = filePathToPseudoNamespace(dependency); | |||
| // Don't output unused variables as that causes TS6133 error | |||
| if (!isUsed(fileDescriptor, pseudoNamespace, exportMap)) { | |||
There was a problem hiding this comment.
This can be moved into the functional iterator. We can write something like
fileDescriptor.getDependencyList()
.filter(dependency => isUsed(fileDescriptor, pseudoNamespace, exportMap))
.forEach((dependency: string) => {...})
| @@ -21,4 +23,11 @@ describe("ts service", () => { | |||
| assert.strictEqual(simple_service_pb_service.SimpleService.DoStream.requestType, simple_service_pb.StreamRequest); | |||
| assert.strictEqual(simple_service_pb_service.SimpleService.DoStream.responseType, external_child_message_pb.ExternalChildMessage); | |||
| }); | |||
| it("should not output imports for namespaces that are not used in the service definition", () => { | |||
There was a problem hiding this comment.
A better solution is to pull
ts-protoc-gen/src/ts/fileDescriptorTSServices.ts
Lines 26 to 34 in 0135ec7
into a separate method which accepts the
fileDescriptor. We can then mock the fileDescriptor and assert the output string doesn't contain the import.
I realize most of that testing style isn't in place so your solution will do for now and we can address it later.
There was a problem hiding this comment.
Agree this is a far better solution and happy to make a subsequent PR to try to improve the tests.
|
@jonnyreeves @jonny-improbable @easyCZ Any plans to do a release now this has been merged? |
|
@jonbretman Yep, will aim pull together a 0.5.0 release this week, tracking in #46 |
I got burnt by #25 too so thought I'd have a go at fixing it.
Will also have a go at adding some tests for this.
@linlexing @MarcusLongmuir