Skip to content

index.d.ts broken#746

Merged
iradul merged 1 commit intoBlizzard:masterfrom
f3nrir92:fix/index.d.ts
Jan 27, 2020
Merged

index.d.ts broken#746
iradul merged 1 commit intoBlizzard:masterfrom
f3nrir92:fix/index.d.ts

Conversation

@f3nrir92
Copy link
Contributor

@f3nrir92 f3nrir92 commented Jan 22, 2020

Package wont be able to compile with ts due to changes in DefinitelyTyped/DefinitelyTyped#40927

Up version after merge pls. This is very critical.

Fix error with EventEmitter caused by DefinitelyTyped/DefinitelyTyped#40927
Add type for KafkaConsumer.createReadStream
Fix return type for Producer.createWriteStream

…#40927

Add type for KafkaConsumer.createReadStream
Fix return type for Producer.createWriteStream
@f3nrir92 f3nrir92 requested review from codeburke and iradul January 22, 2020 21:44
Copy link
Collaborator

@codeburke codeburke left a comment

Choose a reason for hiding this comment

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

I'm not a Typescript expert, but would the EventEmitter change cause any issues with different versions of Node?

@f3nrir92
Copy link
Contributor Author

f3nrir92 commented Jan 23, 2020

Maybe yes if implementation is fully on Typescript. But this is only definitions for type checking at compile time.

Now compilation of Typescript project is broken due to changes in DefinetlyTyped

node_modules/node-rdkafka/index.d.ts:5:29 - error TS2689: Cannot extend an interface 'NodeJS.EventEmitter'. Did you mean 'implements'?

5 export class Client extends NodeJS.EventEmitter {

@iradul
Copy link
Collaborator

iradul commented Jan 24, 2020

The issue happens with @types/node version 13. Node 13 is not officially supported with this library but since the fix is not making any breaking change it's legit.

@iradul iradul merged commit fc8b6b7 into Blizzard:master Jan 27, 2020
@jpdstan
Copy link
Contributor

jpdstan commented Jan 29, 2020

createReadStream is a static property on KafkaConsumer - I think this line is wrong. here's a PR I submitted before with the correct definition: https://github.com/Blizzard/node-rdkafka/pull/745/files

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.

4 participants