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

Conversion of non-promises to promises is problematic for AVA #2

Closed
jamestalmage opened this issue Jan 15, 2016 · 6 comments · Fixed by #3
Closed

Conversion of non-promises to promises is problematic for AVA #2

jamestalmage opened this issue Jan 15, 2016 · 6 comments · Fixed by #3

Comments

@jamestalmage
Copy link

#1 Causes every argument to be converted to a via Promise.resolve(). That creates a small problem for AVA, since this line expects isPromise(fn) to be false if a function was passed in.

Also, the current behavior creates ambiguity, as:

observableToPromise([1,2]);

Produces an identical result to:

observableToPromise(Observable.of(1,2));
@sindresorhus
Copy link
Owner

I thought it was a good idea when I merged #1, but I noticed the issue with AVA too a while ago.

Do you have any suggested solution other than reverting?

// @mastilver

@mastilver
Copy link
Contributor

the Promise.resolve is just there if you pass something which is not an observable

We could just check if the arg is not an observable, neither a function

If you are happy with that, I will create a PR in 5-6 hours

@mastilver
Copy link
Contributor

Or we could only support observable and return the passed arg (as it was before my changes)

then you won't be able to do:

observable-to-promise(2).then(x => {
   console.log(x);
   // => 2
});

will throw Uncaught TypeError: then is not a function

@jamestalmage
Copy link
Author

My only other thought was to throw a TypeError, if they pass a non observable.

It would require AVA do an is-observable test (so things would be a bit more verbose within AVA). However, I think it's a good change because then this API much more clear.

@mastilver
Copy link
Contributor

throw a TypeError

I'm happy with that

@sindresorhus
Copy link
Owner

throw a TypeError

👍

jamestalmage added a commit to jamestalmage/ava that referenced this issue Jan 15, 2016
Observable `0.2.0`/`0.3.0` introduced some breaking changes. Namely, it now throws if passed a non-observable.

Reference: sindresorhus/observable-to-promise#2
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 a pull request may close this issue.

3 participants