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

Update index.d.ts to accept 'finish' flag #177

Merged
merged 2 commits into from
May 20, 2018
Merged

Conversation

brianfitzgerald
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented May 15, 2018

Coverage Status

Coverage remained the same at 99.976% when pulling 0fa908c on brianfitzgerald:patch-1 into 8293a6b on mtth:master.

@@ -21,6 +21,7 @@ export interface CodecOptions {
export interface Decoder {
on(type: 'metadata', callback: (type: Type) => void): this;
on(type: 'data', callback: (value: object) => void): this;
on(type: 'finish', callback: (value: object) => void): this;
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity: Why would you want to listen to the finish event when decoding? Would it not be more logical to listen to the 'end' event? I added that one in my pull request already. If 'finished' is interesting too, we could merge the pull-requests into one...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I can add the end as well. I'm doing this PR since this is causing errors we're having to suppress on production code, and your PR looks like it's pretty comprehensive, so I figured this one would be easier to merge in first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both finished and end are supported by BlockDecoder as it is a Duplex stream. I was just wondering if I should add the Writable events to the BlockDecoder definitions. I noticed the Writable events are not explicitly added to the official Duplex definitions either, that's why I skipped them.

@mtth mtth merged commit dfd8df8 into mtth:master May 20, 2018
@mtth
Copy link
Owner

mtth commented May 20, 2018

Merging this PR first since it's tiny - thanks @brianfitzgerald!

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.

None yet

4 participants