Skip to content

Conversation

@benjie
Copy link

@benjie benjie commented Mar 29, 2018

This PR is to open a dialog; I'm not expecting it to be merged without some significant changes as I've kind of gone at the source with a hatchet... 😇

When a GraphQL server updates the GraphQL schema it would be nice if GraphiQL automatically reflected this. Over at PostGraphile we implement this with a text/event-stream from the server, which fires a change event when the schema changes. Event streams are perfect for this because they're server-sent events, you don't need to worry about receiving data from the client (because you can't) and they are much simpler and more widely supported than websockets.

I'm proposing that we make this a standard - if a GraphQL server supports updating its GraphQL schema it should create the header X-GraphQL-Event-Stream in response to every GraphQL request, passing as the value the relative or absolute URL to the text/event-stream event stream.

The client can then subscribe to the event stream (which can be consumed with EventSource), and will re-inspect the schema whenever a change event is received.

An example of the event-stream implementation in PostGraphile is here:

https://github.com/graphile/postgraphile/blob/master/src/postgraphile/http/setupServerSentEvents.js
https://github.com/graphile/postgraphile/blob/5fd2cf218654e03021ef06ffe8f7556a874906b4/src/postgraphile/http/createPostGraphileHttpRequestHandler.js#L286-L295

I've implemented this in GraphiQL.app in this PR. Though doing so doesn't require much code I went a bit further and made sure that the GraphiQL navigation stack is persisted and fixed some other issues also... At the time I wasn't thinking about sending a PR so it kind of snowballed.

If you're interested in any of this I'm happy to send the changes as separate PRs that can be reviewed individually. I understand reviewing this PR as-is is a lot of work!

PS: a lot of the code in here is MIT licensed by @calebmer; I've modified it to make it work in GraphiQL.app.

🙏 Thanks!

@benjie benjie changed the title Event source [Proposal] X-GraphQL-Event-Stream header Mar 29, 2018
@benjie
Copy link
Author

benjie commented Mar 29, 2018

PPS: apologies that this PR came through before I'd written a description, I accidentally did Cmd-Enter 😳

introspectionQuery,
isType,
GraphQLObjectType,
} from 'graphql'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need pruning

}
]
];
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now guarantee that every tab has a uuid; and move to referring to tabs by UUID to prevent race conditions when tabs open/close.


this.state.tabs.forEach(t => {
this.updateSchema(t.uuid);
});
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we deserialise state we don't have any of the schemas; so go off and fetch them for each tab.

const oldHash = hash(currentTabPreviousState);
const newHash = hash(currentTab);
if (oldHash !== newHash) {
this.debouncedUpdateSchema(currentTab.uuid);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debounced so that typing into the URL bar doesn't queue up tens of network requests and cause lag.

window.localStorage.setItem('tabs', JSON.stringify(this.state.tabs.map(t => {
const { uuid, endpoint, method, headers, name } = t;
return { uuid, endpoint, method, headers, name };
})));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prunes things that we don't want to store to localStorage (namely: the GraphQL schema itself, and the streamUrl which we'll automatically refetch along with the GraphQL schema)

});
}
return this._debouncedUpdateSchema[tabUUID]();
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Need to clean this up on componentDidUpdate to prevent memory leaks

<div className="graphiql-wrapper">
{
// THIS IS THE GROSSEST THING I'VE EVER DONE AND I HATE IT. FIXME ASAP
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you thought yours was gross... We now hack into the internal state of GraphiQL... 😳 🤢

</div>
</div>
: null
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm doing some demo videos and want to reduce unnecessary clutter; please ignore this from this PR - we should probably add an option to hide tabs when there's only one.

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.

1 participant