Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Bring over TS definition from definitelytyped #959

Merged
merged 5 commits into from
May 3, 2018

Conversation

priithaamer
Copy link
Contributor

Simply starting out by bringing over TypeScript definitions from DefinitelyTyped. Fixes to definition file will follow it this PR flies.

See #937

@zoltan-nz
Copy link
Contributor

This looks fantastic. Thank you very much. Great job! :)

What do think about moving the tsconfig.json inside the types folder, where we could have a test file also. Like in the chalk project: https://github.com/chalk/chalk/tree/master/types

And we can somehow extend the npm test also with tsc --project types

Define error parameter in consumer.close callbacks

Fixes SOHU-Co#936
Adds encoding and keyEncoding options that are working with ConsumerGroupOptions.
@priithaamer
Copy link
Contributor Author

Thanks for the recommendation @zoltan-nz. Copied over tests part from DT and added it to test script.

@zoltan-nz
Copy link
Contributor

@priithaamer wow, you were quick, very impressive. Thank you. :)

Looks good to me.

As I see you are an expert in this area. I'm just wondering how would you add the type definition of ProducerStream? I tried it, but the compiler expected, that it should be a kind of WritableStream, and it became quite messy after that. What do you think?

@priithaamer
Copy link
Contributor Author

Ok I will try it. Haven’t used ProducerStream myself so not super familiar with the api.

@zoltan-nz
Copy link
Contributor

@priithaamer finally I figured it out. I just sent a PR to your branch.
priithaamer#1

@priithaamer
Copy link
Contributor Author

@zoltan-nz Nice, thanks! Just had one comment regarding tests. If you are not up to it, i can merge it anyway.

@hyperlink Let me know if anything keeping from merging this.

@hyperlink
Copy link
Collaborator

@priithaamer should we wait to merge until @zoltan-nz changes have been merged to your branch? And I have very limited knowledge of typescript and not sure how to verify this any tips would be helpful.

@priithaamer
Copy link
Contributor Author

priithaamer commented May 2, 2018

I merged the change by @zoltan-nz

Only way to verify is via test task in package.json. There is no good way i know of to test whether type definitions actually match the real code. The next best option is to write tests for the library in TS.

Anyway, it is 99% copied over from @types/kafka-node package and some missing things have been added. So it does not get worse, just easier to use.

@zoltan-nz
Copy link
Contributor

@hyperlink @priithaamer Thank you! I try to extend our test coverage also... let see how is it going. :)

@hyperlink hyperlink merged commit 0521975 into SOHU-Co:master May 3, 2018
@hyperlink
Copy link
Collaborator

Thank you for your contribution!

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

Successfully merging this pull request may close these issues.

3 participants