graphql / graphiql

GraphiQL & the GraphQL LSP Reference Ecosystem for building browser & IDE tools.
MIT License
15.99k stars 1.71k forks source link

Introspection Query is issued even if `schema` prop is provided #2441

Closed 3nvi closed 2 years ago

3nvi commented 2 years ago

Hey there, I'm using the latest 1.9.x version of graphiql and I'm seeing that the introspection query is always fired regardless of whether a schema prop is provided to GraphiQL.

I don't know if this is a new bug or not (or whether I'm missing something here), but I believe that this is the piece of logic that triggers this. Shouldn't there be a check as to whether props.schema is defined before proceeding with an Introspection call?

If so, I'm willing to open a PR to address that. Just let me know if I'm missing something or not.

Cheers!

P.S. For anyone else encountering this, at the moment of writing, the last working version is v1.8.10

3nvi commented 2 years ago

While there, I also found a second bug that relates to the same hook. The headers that get passed over to the fetcher are never updated. They always hold their original value.

The culprit here is this effect. Whenever I change the headers in the secondary editor (regardless of whether their value is controlled or not), the useEffect never fires, cause the headerEditor is always null for me no matter what I've tried here.

Thus the headers.current always holds the initial header value. After an hour's worth of searching I traced it down to the fact that you're attempting to read the EditorContext from within SchemaContextProvider. Funnily though, the SchemaContextProvider wraps the EditorContext and that's why you're not getting any updates there. Reversing the order should fix it, unless the order of the providers was like that for a reason

UPDATE: Just because this is a separate issue, I've opened a new issue here so we can track both bugs separately (and help people find the related issues rather than opening new ones)

acao commented 2 years ago

@thomasheyenbrock looks like we need to fix some issues for the provided schema case. If a user provides a schema, then the schema provider should not try to make a request, and the fetcher is only used for executing operations

thomasheyenbrock commented 2 years ago

Hey @3nvi, thanks for reporting this, this is definitely not intended! I remembered handling this case, but seems like I messed up somewhere.

Feel free to open a PR for this as you already looked into the code. Should be as easy as returning early in this effect if props.schema is truthy or null. (Passing null is effectively running GraphiQL in "schemaless mode".)

(Regarding the headers, I'm gonna answer in the other issue, thanks for keeping things separate 🙏 )

acao commented 2 years ago

@thomasheyenbrock there are a lot of users who override introspection using schema prop, so i would consider this a high priority bug if the introspected schema is overriding the provided schema

3nvi commented 2 years ago

I intend to work on it during the Weekend. If it's something that you want fixed ASAP, perhaps someone else could work on it today

thomasheyenbrock commented 2 years ago

I agree @acao, we should fix that asap! I'm free later today and would fix this and push a release (if @3nvi hasn't started with this yet 😉 )

thomasheyenbrock commented 2 years ago

Just released the fix for this in @graphiql/react@0.2.1 and graphiql@1.9.4