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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 13, 2016
Merged

single interface for fetch #172

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75a1d85
single interface for fetch (observables becomes promises when should …
DxCx Sep 30, 2016
79641ff
fix leebyron CR notes
DxCx Oct 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions src/components/GraphiQL.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ export class GraphiQL extends React.Component {

const fetcher = this.props.fetcher;

const fetch = fetcher({ query: introspectionQuery });
const fetch = observableToPromise(fetcher({ query: introspectionQuery }));
if (!isPromise(fetch)) {
this.setState({
response: 'Fetcher did not return a Promise for introspection.'
Expand All @@ -435,7 +435,9 @@ export class GraphiQL extends React.Component {

// Try the stock introspection query first, falling back on the
// sans-subscriptions query for services which do not yet support it.
const fetch2 = fetcher({ query: introspectionQuerySansSubscriptions });
const fetch2 = observableToPromise(fetcher({
query: introspectionQuerySansSubscriptions
}));
if (!isPromise(fetch)) {
throw new Error('Fetcher did not return a Promise for introspection.');
}
Expand Down Expand Up @@ -895,6 +897,23 @@ function isPromise(value) {
return typeof value === 'object' && typeof value.then === 'function';
}

// Duck-type Observable.take(1).toPromise()
function observableToPromise(observable) {
if ( !isObservable(observable) ) {
Copy link
Contributor

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?

Copy link
Contributor Author

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..

Copy link
Contributor

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.

return observable;
}
return new Promise((resolve, reject) => {
Copy link
Contributor

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;

Copy link
Contributor Author

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

Copy link
Contributor

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()

Copy link
Contributor Author

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. :)

const subscription = observable.subscribe(v => {
resolve(v);
subscription.unsubscribe();
}, e => {
reject(e);
}, () => {
reject(new Error('no value resolved'));
});
});
}

// Duck-type observable detection.
function isObservable(value) {
return typeof value === 'object' && typeof value.subscribe === 'function';
Expand Down