Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat!: add support for sharded pubsub topics & remove support for named pubsub topics #1697
feat!: add support for sharded pubsub topics & remove support for named pubsub topics #1697
Changes from 3 commits
5d59a0b
6ae6fd6
a7d7673
9d3e34b
9c88102
b6ea40a
85a786a
a0de535
973ab4f
ab4831c
fe81a26
7fd07b2
85500d3
0579376
f42d848
5dd508c
d10eb95
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Topic and Shard are redundant. It's a single shard info, which is also a single topic info.
The redundancy of information makes the interface name more complicated for no added value.
Also, you break the interface with
ShardInfo
here as you removed the index list.Considering you only bring one property (
cluster
) over fromShardInfo
, I am not sure to understand the point of extending fromShardInfo
. Just create a new type.Re-using the type is only useful if you want a function that accept
ShardInfo
to also acceptSingleTopicShardInfo
. But because of theOmit
, you are not getting that.If that's what you want, then try to override the
indexList
with a tuple type to specify that it's an array with only onenumber
.If you don't need interface compatibility between
ShardInfo
andSingleTopicShardInfo
, then don't make the latter extend the former, just redefine a new interface.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point that since the two interfaces don't share a lot of properties (have just one in common of the two), thus it might make more sense to define a new interface.
Omit
was used because I wanted to showcase the link between the two types, while keeping one property. When is a case you thinkOmit
would make sense for a usecase like this here? Perhaps when there are more number of properties?--
Addressed redefining the interface here: 5dd508c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO
Omit
makes sense when you want common API to be used. e.g.I see your point re code documentation and duplication but with so very few properties, I think it makes it less readable.
Check warning on line 223 in packages/sdk/src/create.ts
GitHub Actions / check
Check warning on line 223 in packages/sdk/src/create.ts
GitHub Actions / proto