Skip to content

Expose MethodDescriptor's public methods - #1160

Merged
sampajano merged 6 commits into
grpc:masterfrom
tomferreira:master
Nov 19, 2021
Merged

Expose MethodDescriptor's public methods#1160
sampajano merged 6 commits into
grpc:masterfrom
tomferreira:master

Conversation

@tomferreira

@tomferreira tomferreira commented Nov 3, 2021

Copy link
Copy Markdown
Contributor

Expose the public methods of the MethodDescriptor class to be used in a mock implementation proposal of interceptor.

@linux-foundation-easycla

linux-foundation-easycla Bot commented Nov 3, 2021 •
edited
Loading

Copy link
Copy Markdown

CLA Signed

The committers are authorized under a signed CLA.

sampajano
sampajano reviewed Nov 8, 2021
View reviewed changes

@sampajano sampajano left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the contrib! :)

This in principle is fine with me. I'm a bit surprised that these aren't exported already given it's already in the TS interface.

@stanley-cheung would probably have more context here so i'll wait for his review :)

(BTW you need to finish the CLA process for this PR to be merge-able :))

stanley-cheung
stanley-cheung approved these changes Nov 8, 2021
View reviewed changes

@stanley-cheung stanley-cheung left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This LGTM. Yea MethodDescriptor should be part of the public API.

Comment thread packages/grpc-web/externs.js Outdated
@sampajano sampajano mentioned this pull request Nov 9, 2021
Closed
sampajano
sampajano approved these changes Nov 17, 2021
View reviewed changes

@sampajano sampajano left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks again for the change! :)

@sampajano
sampajano merged commit 97baed4 into grpc:master Nov 19, 2021
@sampajano

sampajano commented Feb 1, 2022

Copy link
Copy Markdown
Collaborator

Hi @tomferreira.. Unfortunately i have to rollback this PR because after merging this change it's causing some non-trivial code size increases for some internal Google products.. Apologize for the inconveniences.. 😅

I think maybe there are some ways to expose these APIs ONLY for Github. But I'm not totally sure yet. Ideas are welcome too.. 😃

sampajano added a commit that referenced this pull request Feb 1, 2022
@sampajano
Revert "Expose MethodDescriptor's public methods (#1160)"
eba56cf 
This reverts commit 97baed4.
@sampajano sampajano mentioned this pull request Feb 1, 2022
Merged
sampajano added a commit that referenced this pull request Feb 1, 2022
@sampajano
Revert "Expose MethodDescriptor's public methods (#1160)" (#1199)
81ce4c5 
This reverts commit 97baed4.
@sampajano sampajano mentioned this pull request Sep 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sampajano sampajano sampajano approved these changes
@stanley-cheung stanley-cheung stanley-cheung approved these changes

Assignees

No one assigned

Labels

release notes: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tomferreira @sampajano @stanley-cheung @grpc-kokoro