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

Export the Stream class so type information can be imported #3456

Merged
merged 1 commit into from
Dec 8, 2020

Conversation

sholtrop
Copy link
Contributor

@sholtrop sholtrop commented Dec 7, 2020

In TypeScript, I would like to be able to do this:

let inferenceStream: Deepspeech.Stream | null = null

To then initialize it later. However, the Stream class is not exported, and as a result neither is its type definition, meaning I cannot import it. This PR just adds the export keyword to solve this.

@community-tc-integration
Copy link

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

@lissyx
Copy link
Collaborator

lissyx commented Dec 7, 2020

@reuben it feels like this is something we, want to do, right ?

@sholtrop i know nothing about typescript, what would be the consequences ?

@sholtrop
Copy link
Contributor Author

sholtrop commented Dec 7, 2020

Thanks for the quick reply!
Exporting it means the class as well as its type information can be imported/used by users of the library.
The only downside is that the Stream class is not meant to be instantiated by users directly (as is noted in the docs). This will make it possible for users to do so erroneously.
Exporting ONLY the type information is also possible, if that is preferred.

@lissyx
Copy link
Collaborator

lissyx commented Dec 7, 2020

Exporting ONLY the type information is also possible, if that is preferred.

Yes, I think it would be better in light of that. Can you update your PR?

@sholtrop
Copy link
Contributor Author

sholtrop commented Dec 7, 2020

I've named its exported type StreamType. This means this now works:

import {StreamType, Model} from './native_client/javascript'

const model = new Model(`path/to/model`)
let stream: StreamType | null = null
if (true)
  stream = model.createStream()

@reuben
Copy link
Contributor

reuben commented Dec 8, 2020

@reuben it feels like this is something we, want to do, right ?

Yep.

@reuben
Copy link
Contributor

reuben commented Dec 8, 2020

I think we should flip the naming, export type Stream and name the class StreamImpl (and update its users)

@lissyx
Copy link
Collaborator

lissyx commented Dec 8, 2020

@sholtrop After you switch to StreamImpl, please make sure you have squashed all your commits into one on the PR, then we'll run CI to make sure. Thanks for spotting that!

@sholtrop
Copy link
Contributor Author

sholtrop commented Dec 8, 2020

Done! Hope I did everything correctly

@reuben
Copy link
Contributor

reuben commented Dec 8, 2020

Opened #3457 to run tests.

@reuben reuben merged commit 8c8387c into mozilla:master Dec 8, 2020
@lissyx
Copy link
Collaborator

lissyx commented Dec 8, 2020

@reuben Maybe it's worth merging this also on r0.9? A 0.9.3 could benefit from this, too bad we shipped 0.9.2 a few days ago :)

@reuben
Copy link
Contributor

reuben commented Dec 8, 2020

Yeah I think so.

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.

3 participants