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

feat(addListener): throw an error if next, error or complete functions are missing #12

Closed
wants to merge 3 commits into from

Conversation

Widdershin
Copy link
Contributor

This class of error is a compiler error in TypeScript, but is a runtime error in JS. Currently we get an error like "this._n is not a function" which is quite cryptic to an end user.

I would add a test for this, but since it's a compiler error in TS I can't see any way to do so.

…s are missing

This class of error is a compiler error in TypeScript, but is a runtime
error in JS. Currently we get an error like "this._n is not a function"
which is quite cryptic to an end user.
@staltz
Copy link
Owner

staltz commented Apr 26, 2016

I would add a test for this, but since it's a compiler error in TS I can't see any way to do so.

Here's a dirty trick I sometimes use:

var listener = <Listener<T>> <any> incompleteListener;

👯

@ghost
Copy link

ghost commented Apr 26, 2016

Does this mean that the end-user won't be able to subscribe to a stream without providing _all_ 3 functions (onNext, onError, onCompleted)?

@Widdershin
Copy link
Contributor Author

That is correct, and is in fact by design. See #1.

@ghost
Copy link

ghost commented Apr 26, 2016

@Widdershin thanks for clearing it up.

done.fail isn't a thing, but if you call done with any value it will
treat it as an error, so this works just fine.
@Widdershin
Copy link
Contributor Author

@staltz fixed the failures (which were the result of done.fail being undefined) and added some tests for these errors. I feel like this should be good to merge 😺

@ghost
Copy link

ghost commented Apr 27, 2016

👍

@staltz
Copy link
Owner

staltz commented Apr 27, 2016

Merged as of 10721b6, thanks @Widdershin. I ended up trimming down the code, for size.

@staltz staltz closed this Apr 27, 2016
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