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

Add functions for loading and parsing binary-encoded or plain object file descriptor sets #1635

Merged
merged 5 commits into from
Dec 1, 2020
Merged

Add functions for loading and parsing binary-encoded or plain object file descriptor sets #1635

merged 5 commits into from
Dec 1, 2020

Conversation

tatemz
Copy link
Contributor

@tatemz tatemz commented Nov 22, 2020

Fixes #1627, #550, #807, and #1535

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 22, 2020

CLA Signed

The committers are authorized under a signed CLA.

@@ -368,6 +379,38 @@ export function loadSync(
return createPackageDefinition(root, options!);
}

export async function loadFileDescriptorSet(
descriptorSet: Protobuf.Message<descriptor.IFileDescriptorSet> &
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a Message, the input here should be a Buffer containing the binary-encoded message, which should then be decoded within the function. Alternatively, or in addition, the input could be a plain JavaScript object that could be passed to Protobuf.Message<descriptor.IFileDescriptorSet>.fromObject().

As I mentioned in the issue, hiding the Protobuf.js API is a core design goal of this library. That means that the user should not need to have any interaction with the Protobuf.js library directly in order to use any of the APIs exported from this library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Thanks for the advice.

@@ -368,6 +379,38 @@ export function loadSync(
return createPackageDefinition(root, options!);
}

export async function loadFileDescriptorSet(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the operations in this function are async, so the function itself does not need to be async. I would prefer to make it synchronous in that case for flexibility.

This change refactors the loadFileDescriptorSetFile function to take
a plain Buffer or Javascript object as input.
@tatemz tatemz changed the title Add functions for loading and parsing binary-encoded file decriptor sets Add functions for loading and parsing binary-encoded or plain object file decriptor sets Nov 24, 2020
@tatemz tatemz changed the title Add functions for loading and parsing binary-encoded or plain object file decriptor sets Add functions for loading and parsing binary-encoded or plain object file descriptor sets Nov 24, 2020
This change ensures that errors are not hidden when
loadFileDescriptorSet fails to decode JSON. Instead, only JSON parsing
errors are hidden.
export function loadFileDescriptorSet(
descriptorSet:
| Buffer
| ReturnType<typeof descriptor.FileDescriptorSet.toObject>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two different kinds of arguments here should probably just be different functions. They're addressing different use cases.

Also, toObject and fromObject aren't full inverses, so it would be more accurate to use Parameters<typeof descriptor.FileDescriptorSet.fromObject>[0].

return createPackageDefinition(root, options);
}

export function loadFileDescriptorSetFile(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this function is necessary. Its functionality is just "Read a file and pass the contents through a different exported function". That's not specific to this library, so I think we should just let the user determine where the data comes from.

That contrasts with the load and loadSync functions, in which dependency resolution across files is an essential part of their functionality.

This change exposes loadFileDescriptorSetFromBuffer and
loadFileDescriptorSetFromObject functions.
Copy link
Member

@murgatroid99 murgatroid99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting this together.

@murgatroid99 murgatroid99 merged commit 604dd0f into grpc:master Dec 1, 2020
@tatemz tatemz deleted the feature/load-file-descriptor-sets branch December 1, 2020 20:02
@ardatan
Copy link

ardatan commented Dec 9, 2020

Do you have an estimated time to release this? Thanks!

@murgatroid99
Copy link
Member

I was taking time off last month, so I did not release anything during that time. I should be able to release this PR this week.

@ardatan
Copy link

ardatan commented Jan 14, 2021

@murgatroid99 Any updates?

@murgatroid99
Copy link
Member

Sorry about that, this got lost in the shuffle with other things I was working on. I don't like doing Friday releases, so I'll get this out next week.

@ardatan
Copy link

ardatan commented Jan 16, 2021

Thanks 🙏

@ardatan
Copy link

ardatan commented Jan 19, 2021

Just reminder :) Sorry for bugging you @murgatroid99

@murgatroid99
Copy link
Member

This is now out in version 0.5.6

@ardatan
Copy link

ardatan commented Jan 20, 2021

Thank you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reopen: Support parsing of file descriptor sets into package definitions
3 participants