-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
single interface for fetch #172
Conversation
I'm concerned that this will break functionality for subscriptions which use Observables to provide multiple payloads for a single request over time. Could you clarify what the problem/bug is that you're trying to solve? |
if ( isPromise(observable) ) { | ||
return observable; | ||
} | ||
if ( !isObservable(observable) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why both conditions above? Are both needed or is only one sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the second, the first is more for readability.
If you want ill remove it..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - only the second is necessary.
Actually ive written a fetch function which returns observable (and hence, tested it with it) and the issue is that the schema cannot be observable only promise, so, no, its not breaking its just means that the schema returned can be observable which emits one value. As you know from the other thread im creating a full flow example of subscriptions using observables and this a small issue ive found on the way. (Fetch function expects to return both promise and observables on different places) |
Ah thanks for the extra info. I see you're only wrapping the schema fetching so this looks good |
if ( !isObservable(observable) ) { | ||
return observable; | ||
} | ||
return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you actually want the first value, a "takeOne" kind of logic, rather than waiting for the last. If fetcher returned some sort of unclosing socket, this could never complete.
Perhaps:
if (isObservable(promiseOrObservable)) {
return new Promise((resolve, reject) => promiseOrObservable.subscribe(resolve, reject, reject));
}
return promiseOrObservable;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont mind changing it, the reason it is done that way is to be aligned with toPromise() operator which is waiting for last value.
Because of that it is often used with .take(1) operator before it.
Let me know if you want to change the behavior of the function or keep it aligned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we want a sort of inlined .take(1).toPromise()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, Gotcha, so in that case unsubscribe should be called as well.
also, reject won't give any helpful information.
I've added a commit with both CR notes. :)
This lgtm as well - thanks for the contribution! |
since the fetcher shouldn't know if it is expected to return promise or observable,
it added a small logic to convert observable into promises when promise is a must.