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

Topic Offset Mismatch Between Documentation and Types #978

Closed
ccarcaci opened this issue Dec 1, 2020 · 3 comments
Closed

Topic Offset Mismatch Between Documentation and Types #978

ccarcaci opened this issue Dec 1, 2020 · 3 comments
Labels

Comments

@ccarcaci
Copy link
Contributor

ccarcaci commented Dec 1, 2020

Describe the bug
There's a mismatch between the documentation about the seek method here:
https://kafka.js.org/docs/consuming#a-name-seek-a-seek
And the related type of the function:
https://github.com/tulios/kafkajs/blob/master/types/index.d.ts#L843

The documentation says that the offset should be a number:
consumer.seek({ topic: 'example', partition: 0, offset: 12384 })

But the seek method type has a signature that requires a string:
seek(topicPartition: { topic: string; partition: number; offset: string }): void

To Reproduce
No need to reproduce the error.

Expected behavior
offset type should be a number.

Observed behavior
offset is a string.

@tulios
Copy link
Owner

tulios commented Dec 1, 2020

Yes, we need to improve that. The TL;DR is that offset is an int64, and we use strings to represent those values, so you don't lose anything on precision issues. Internally we convert the input to our internal representation of a BigInt, so the number and the string inputs will work. Accepting the number is just a quality of life improvement on the developer experience. Most of the types came from community contributions, and perhaps the contributor never reached offsets greater than an int32. We should update the definition to accept both types, and I think we also accept BigInt there nowadays.

Do you think you could create a PR to add string | number?

@tulios tulios added the question label Dec 1, 2020
@ccarcaci
Copy link
Contributor Author

ccarcaci commented Dec 2, 2020

Of course, I can do that.
Using typescript I can't specify numbers because the compiler won't allow that.
In any case my PR is coming.
Thank you

@ccarcaci
Copy link
Contributor Author

ccarcaci commented Dec 2, 2020

PR created from a forked project.
#981
Thank you :)

@tulios tulios closed this as completed Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants