Skip to content
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

[BUG] The generate_dependencies parameter doesn't work #59

Closed
jcready opened this issue Jan 3, 2021 · 4 comments
Closed

[BUG] The generate_dependencies parameter doesn't work #59

jcready opened this issue Jan 3, 2021 · 4 comments
Labels
bug Something isn't working

Comments

@jcready
Copy link
Contributor

jcready commented Jan 3, 2021

When debugging the issue I was able to narrow it down to this method:

isTypeUsed(type: AnyTypeDescriptorProto, inFiles: FileDescriptorProto[]): boolean {
let used = false;
for (let fd of inFiles) {
this.tree.visitTypes(fd, typeDescriptor => {
if (used) return;
if (DescriptorProto.is(typeDescriptor)) {
const usedInField = typeDescriptor.field.includes(type);
if (usedInField) {
used = true;
}
} else if (ServiceDescriptorProto.is(typeDescriptor)) {
const usedInMethodInput = typeDescriptor.method.some(md => this.nameLookup.resolveTypeName(md.inputType!) === type);
const usedInMethodOutput = typeDescriptor.method.some(md => this.nameLookup.resolveTypeName(md.outputType!) === type);
if (usedInMethodInput || usedInMethodOutput) {
return true;
}
}
})
}
return used;
}

I was able to resolve the issue by changing the method to do the following:

 isTypeUsed(type: AnyTypeDescriptorProto, inFiles: FileDescriptorProto[]): boolean {
     // Probably not the right way to get this
     const normalizedTypeName = this.nameLookup._reverse.get(type);
     let used = false;
     for (let fd of inFiles) {
         if (used) return used;
         this.tree.visitTypes(fd, typeDescriptor => { 
             if (used) return;
             if (DescriptorProto.is(typeDescriptor)) {
                 const usedInField = typeDescriptor.field
                     .map(f => f.typeName)
                     .filter(Boolean)
                     .map(this.nameLookup.normalizeTypeName)
                     .includes(normalizedTypeName);
                 if (usedInField) {
                     used = true;
                 }
             } else if (ServiceDescriptorProto.is(typeDescriptor)) { 
                 const usedInMethodInput = typeDescriptor.method.some(md => this.nameLookup.resolveTypeName(md.inputType!) === type); 
                 const usedInMethodOutput = typeDescriptor.method.some(md => this.nameLookup.resolveTypeName(md.outputType!) === type); 
                 if (usedInMethodInput || usedInMethodOutput) { 
                     used = true; 
                 } 
             } 
         }) 
     } 
     return used; 
 } 

I'd be happy to open a PR if you think this is the correct way to resolve the issue.

@timostamm
Copy link
Owner

Good catch, @jcready. PR is welcome!

A test case would be nice (but not required), see descriptor-registry.spec.ts for example.

    // Probably not the right way to get this
    const normalizedTypeName = this.nameLookup._reverse.get(type);

You should be able to use makeTypeName:

    const normalizedTypeName = this.nameLookup.makeTypeName(type);

@timostamm timostamm added the bug Something isn't working label Jan 6, 2021
@timostamm
Copy link
Owner

Hey @jcready, I just added your fix. Hope you don't mind, and thank you.

@jcready
Copy link
Contributor Author

jcready commented Jan 19, 2021

No worries! I've been a bit busy with other things to make time for a proper PR with test coverage. Thank you for fixing the issue and the excellent library!

@timostamm
Copy link
Owner

Released in v2.0.0-alpha.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants