From 554b7fc6b3c5be2a89b6fbc698fa9fd1fedde46f Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Tue, 1 Dec 2015 15:49:13 -0800 Subject: [PATCH] [FIX] bound fetcher function could cause infinite loop This fixes an issue introduced by 82b059d51537b5c8c785b2a2f5c8b1db033b495c which refetched schema when the fetching function changes. If the GraphiQL React component is provided with a bound function, like `this.fetcher.bind(this)` then every time it is rendered, it will fetch a new schema. If the result of fetching the schema causes any changes which trigger a rerender, this can result in an infinite loop. This behavior is removed by this diff, so providing a new fetching function no longer refetches the schema. In addition, this also solves a race condition where a provided schema would be replaced by the pending fetched schema. In practice, looking for a different fetching function is also insufficient for the original goal of refetching a schema when the backing server has changed, as that bit of information could very possibly be held in some other encapsulated state, which would not result in function identity changing. To actually satisfy the original goal, this provides the ability for setting the schema (and also query and variables) to `null` to represent no value, whereas the value `undefined` continues to be interpretted as not provided, falling back to the existing default behaviors. For installations of GraphiQL which seek to swap out different fetching functions and schemas, it will work best if the schema is created and managed outside of GraphiQL's default behavior. --- README.md | 24 +++++---- src/components/GraphiQL.js | 107 +++++++++++++++++++------------------ 2 files changed, 68 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index fd98c2e2cfe..428734de5d8 100644 --- a/README.md +++ b/README.md @@ -67,14 +67,20 @@ and children. **Props:** -- `fetcher`: a required function which accepts GraphQL-HTTP parameters and - returns a Promise which resolves to the GraphQL parsed JSON response. +- `fetcher`: a function which accepts GraphQL-HTTP parameters and returns + a Promise which resolves to the GraphQL parsed JSON response. -- `schema`: an optional GraphQLSchema instance. If one is not provided, - GraphiQL will fetch one using introspection. +- `schema`: a GraphQLSchema instance or `null` if one is not to be used. + If `undefined` is provided, GraphiQL will send an introspection query + using the fetcher to produce a schema. - `query`: an optional GraphQL string to use as the initial displayed query, - if not provided, the local storage or defaultQuery will be used. + if `undefined` is provided, the stored query or defaultQuery will + be used. + +- `variables`: an optional GraphQL string to use as the initial displayed + query variables, if `undefined` is provided, the stored variables will + be used. - `response`: an optional JSON string to use as the initial displayed response. If not provided, no response will be initialy shown. You might @@ -83,11 +89,9 @@ and children. - `storage`: an instance of [Storage][] GraphiQL will use to persist state. Only `getItem` and `setItem` are called. Default: window.localStorage -- `defaultQuery`: an optional GraphQL string to use instead of a - blank screen when a query was not found in the local cache. - -- `variables`: an optional GraphQL string to use as the initial displayed - query variables, if not provided, the local storage will be used. +- `defaultQuery`: an optional GraphQL string to use when no query is provided + and no stored query exists from a previous session. If `undefined` is + provided, GraphiQL will use its own default query. - `onEditQuery`: an optional function which will be called when the Query editor changes. The argument to the function will be the query string. diff --git a/src/components/GraphiQL.js b/src/components/GraphiQL.js index ed7fd01038a..9f83092087a 100644 --- a/src/components/GraphiQL.js +++ b/src/components/GraphiQL.js @@ -34,11 +34,17 @@ import { * - fetcher: a function which accepts GraphQL-HTTP parameters and returns * a Promise which resolves to the GraphQL parsed JSON response. * - * - schema: an optional GraphQLSchema instance. If one is not provided, - * GraphiQL will fetch one using introspection. + * - schema: a GraphQLSchema instance or `null` if one is not to be used. + * If `undefined` is provided, GraphiQL will send an introspection query + * using the fetcher to produce a schema. * * - query: an optional GraphQL string to use as the initial displayed query, - * if not provided, the stored query or defaultQuery will be used. + * if `undefined` is provided, the stored query or defaultQuery will + * be used. + * + * - variables: an optional GraphQL string to use as the initial displayed + * query variables, if `undefined` is provided, the stored variables will + * be used. * * - response: an optional JSON string to use as the initial displayed * response. If not provided, no response will be initialy shown. You might @@ -47,11 +53,9 @@ import { * - storage: an instance of [Storage][] GraphiQL will use to persist state. * Only `getItem` and `setItem` are called. Default: window.localStorage * - * - defaultQuery: an optional GraphQL string to use instead of a - * blank screen when a query was not found in the local cache. - * - * - variables: an optional GraphQL string to use as the initial displayed - * query variables, if not provided, the stored variables will be used. + * - defaultQuery: an optional GraphQL string to use when no query is provided + * and no stored query exists from a previous session. If `undefined` is + * provided, GraphiQL will use its own default query. * * - onEditQuery: an optional function which will be called when the Query * editor changes. The argument to the function will be the query string. @@ -140,7 +144,7 @@ export class GraphiQL extends React.Component { // Lifecycle constructor(props) { - super(); + super(props); // Ensure props are correct if (typeof props.fetcher !== 'function') { @@ -151,14 +155,16 @@ export class GraphiQL extends React.Component { this._storage = props.storage || window.localStorage; // Determine the initial query to display. - var query = - props.query || - this._storageGet('query') || - props.defaultQuery || + const query = + props.query !== undefined ? props.query : + this._storageGet('query') !== undefined ? this._storageGet('query') : + props.defaultQuery !== undefined ? props.defaultQuery : defaultQuery; // Determine the initial variables to display. - var variables = props.variables || this._storageGet('variables'); + const variables = + props.variables !== undefined ? props.variables : + this._storageGet('variables'); // Initialize state this.state = { @@ -179,24 +185,20 @@ export class GraphiQL extends React.Component { } componentWillReceiveProps(nextProps) { - var nextSchema = this.state.schema; - var nextQuery = this.state.query; - var nextVariables = this.state.variables; - var nextResponse = this.state.response; - if (nextProps.schema && nextProps.schema !== nextSchema) { + let nextSchema = this.state.schema; + let nextQuery = this.state.query; + let nextVariables = this.state.variables; + let nextResponse = this.state.response; + if (nextProps.schema !== undefined) { nextSchema = nextProps.schema; - } else if (!nextProps.schema && nextProps.fetcher !== this.props.fetcher) { - // If a new fetcher is provided, but no schema is provided, remove any - // existing schema. - nextSchema = null; } - if (nextProps.query && nextProps.query !== nextQuery) { + if (nextProps.query !== undefined) { nextQuery = nextProps.query; } - if (nextProps.variables && nextProps.variables !== nextVariables) { + if (nextProps.variables !== undefined) { nextVariables = nextProps.variables; } - if (nextProps.response && nextProps.response !== nextResponse) { + if (nextProps.response !== undefined) { nextResponse = nextProps.response; } this.setState({ @@ -209,18 +211,36 @@ export class GraphiQL extends React.Component { componentDidMount() { // If there is no schema provided via props, fetch one using introspection. - if (!this.state.schema) { - this._fetchSchema(); + if (this.state.schema !== undefined) { + return; } + + const fetcher = this.props.fetcher; + + // Try the stock introspection query first, falling back on the + // sans-subscriptions query for services which do not yet support it. + fetcher({ query: introspectionQuery }) + .catch(() => fetcher({ query: introspectionQuerySansSubscriptions })) + .then(result => { + // If a schema was provided while this fetch was underway, then + // satisfy the race condition by respecting the already + // provided schema. + if (this.state.schema !== undefined) { + return; + } + + if (result.data) { + this.setState({ schema: buildClientSchema(result.data) }); + } else { + this.setState({ response: JSON.stringify(result, null, 2) }); + } + }) + .catch(error => { + this.setState({ response: error && (error.stack || String(error)) }); + }); } componentDidUpdate(prevProps, prevState) { - // If there is no schema provided via props, and a new fetcher is provided, - // fetch one using introspection. - if (!this.state.schema && this.props.fetcher !== prevProps.fetcher) { - this._fetchSchema(); - } - // When UI-altering state changes, simulate a window resize event so all // CodeMirror instances become properly rendered. if (this.state.variableEditorOpen !== prevState.variableEditorOpen || @@ -334,25 +354,6 @@ export class GraphiQL extends React.Component { this._storage.setItem('graphiql:' + name, value); } - _fetchSchema() { - var fetcher = this.props.fetcher; - - // Try the stock introspection query first, falling back on the - // sans-subscriptions query for services which do not yet support it. - fetcher({ query: introspectionQuery }) - .catch(() => fetcher({ query: introspectionQuerySansSubscriptions })) - .then(result => { - if (!result.data) { - this.setState({ response: JSON.stringify(result, null, 2) }); - } else { - this.setState({ schema: buildClientSchema(result.data) }); - } - }) - .catch(error => { - this.setState({ response: error && (error.stack || String(error)) }); - }); - } - _fetchQuery(query, variables, cb) { this.props.fetcher({ query, variables }).then(cb).catch(error => { this.setState({