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

Correct Client.stream return type: from EventEmitter to ResultStream #405

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

p-flock
Copy link

@p-flock p-flock commented Aug 24, 2023

The client implementation of the stream method returns a ResultStream. This commit corrects the type listed in the index.ts file to line up with the implementation

The client implementation of the `stream` method returns a `ResultStream`. This commit corrects the type listed in the index.ts file to line up with the implementation
@p-flock
Copy link
Author

p-flock commented Aug 24, 2023

Just pre-emptively saying I have signed the datastax CLA

@absurdfarce
Copy link
Collaborator

Thanks for the PR @p-flock! And for the pre-emptive note about the CLA. ;)

With regard to the change itself, the intent of the stream() interface is to provide an easy entry point for handlers of streaming events along the lines of what's laid out in the documentation. I could perhaps see a case for making the returned type stream.Readable in order to expose some additional context but it's not immediately clear to me that you buy much by going a step further and marking that function as returning types.ResultStream. Is there some specific bit of functionality on types.ResultStream that you're looking to expose? I'm not opposed to the idea necessarily.. I'm just trying to get a better handle on your use case in order to understand the proposed change.

Thanks again!

@p-flock
Copy link
Author

p-flock commented Sep 18, 2023

Thanks for the PR @p-flock! And for the pre-emptive note about the CLA. ;)

With regard to the change itself, the intent of the stream() interface is to provide an easy entry point for handlers of streaming events along the lines of what's laid out in the documentation. I could perhaps see a case for making the returned type stream.Readable in order to expose some additional context but it's not immediately clear to me that you buy much by going a step further and marking that function as returning types.ResultStream. Is there some specific bit of functionality on types.ResultStream that you're looking to expose? I'm not opposed to the idea necessarily.. I'm just trying to get a better handle on your use case in order to understand the proposed change.

Thanks again!

thanks for following up @absurdfarce , sorry for the delayed response.

The change I'm hoping to make is not actually to the implementation, just to correct the type that is specified in the index.ts file.

the client implementation of stream
has @returns {ResultStream} in it's docblock, and that matches the implementation below.

what I believe is incorrect is the index.d.ts file which lists the return type of this method as events.EventEmitter which causes unexpected type errors for typescript libraries or applications which rely on that method.

For example in my application I cast the result of Client.streamcalls with as unknown as Readable, because that matches the runtime behavior of the the datastax driver.

It's possible I'm mistaken and there is good reason for the type specified in the definition file to be events.EventEmitter, but when I first came across this it seemed like a simple issue of the type definition file not being maintained along with library updates, since this library is not actually compiled with typescript it is very understandable for the mismatch to go unnoticed.

This change would also make the listed type make more sense alongside other Client class methods, e.g. execute which has it's return type listed as Promise<types.ResultSet> in the index.d.ts file, matching the doc-block and implementation in the library code.

As you mentioned, it would also be correct to have the specified return type be stream.Readable, and would be just as functional for library users. I figured I would stay in line with the other methods convention of using the internal type, in this case types.ResultStream, but happy to amend my commit to use stream.Readable if you would prefer.

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.

2 participants