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

Fix ClientOptions types #1559

Merged

Conversation

smolijar
Copy link
Contributor

@smolijar smolijar commented Sep 1, 2020

Remove index signature from ChannelOptions to fix intersection error
described in #1558 which causes issues on using ClientOptions direct
fields with TypeScript.

Removing of index signature required few minor changes:

  • adding few constant types that were used throughout the app
  • using as const assertion in xds-client
  • using not-so-great type cast in channelOptionsEqual

Alternative solution would be removing the index signature from
ChannelOptions explicitly in ClientOptions definition, which is not
trivial and probably calls for a generic type helper.

See: #1558
Fixes: 1558

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 1, 2020

CLA Check
The committers are authorized under a signed CLA.

@murgatroid99
Copy link
Member

I think the correct solution here is actually to just make the index signature have the value type any. It's important to allow unspecified keys to break equality because that is how subchannel sharing is determined.

@smolijar smolijar force-pushed the fix/1558-broken-client-option-types branch from 3301e60 to 56241fe Compare September 1, 2020 19:46
Remove index signature from ChannelOptions to fix intersection error
described in grpc#1558 which causes issues on using ClientOptions direct
fields with TypeScript.

Removing of index signature required few minor changes:
 - adding few constant types that were used throughout the app
 - using `as const` assertion in xds-client
 - using not-so-great type cast in channelOptionsEqual

Alternative solution would be removing the index signature from
ChannelOptions explicitly in ClientOptions definition, which is not
trivial and probably calls for a generic type helper.

See: grpc#1558
Fixes: grpc#1558
@smolijar smolijar force-pushed the fix/1558-broken-client-option-types branch from 56241fe to b2d8982 Compare September 1, 2020 19:48
@smolijar
Copy link
Contributor Author

smolijar commented Sep 1, 2020

@murgatroid99 Thank you for feedback, I am not too familiar with the codebase. I added the index any signature and removed the type obscurities derived from the previous solution.

I justk kept the addition of previously added keys apart from that, which are used throughout the codebase.

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 your contribution.

@murgatroid99 murgatroid99 merged commit e0b19e5 into grpc:master Sep 2, 2020
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.

None yet

3 participants