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.

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

Don't unnecessarily invoke type loader when printing schema #1336

Closed
wants to merge 1 commit into from

Conversation

mfn
Copy link
Contributor

@mfn mfn commented Mar 5, 2023

Summary

Before #1303 , the type loader was not invoked when printing the schema and not mutation or subscription is defined.

This brings back the previous behaviour in this regard.

I'm not sure what is really wanted here, it was discovered as feedback in the preparation for graphql-laravel for the graphql-php v15 upgrade in rebing/graphql-laravel#953 (comment) and we added a test case which did not trigger the typeloader in v14 ( rebing/graphql-laravel#995 , though at that point it wasn't yet clear what's going on).

It's not a big issue, but I figured I open the PR as discussion, if we really need to invoke the type loader if we already see that there's not such top level type defined.

The test is just quickly put together: if you change back to the code in master, the test throws an exception.

Let me know what you think, cheers.

@mfn
Copy link
Contributor Author

mfn commented Mar 5, 2023

Should have run the full test suite locally before I guess 😅

I see what it breaks, though not sure why with my change. But I guess that's a reason to no go forward with this PR.

@spawnia
Copy link
Collaborator

spawnia commented Mar 5, 2023

The type loader should not throw when a type is not found. Such behaviour would be problematic because types can be requested by the client, e.g. in a fragment spread.

@mfn
Copy link
Contributor Author

mfn commented Mar 5, 2023

Thanks!

I wasn't aware of this, the exception in graphql-laravel predates my involvement by a couple of years and was added in the original fork before I even knew what GraphQL was 😅.

I'm closing this PR as I'm sure this is just unnecessary noise and I'll consider your advice, thank you 🙏🏼

@mfn mfn closed this Mar 5, 2023
@mfn mfn deleted the mfn-schema-print-typeloader branch March 5, 2023 18:25
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.

None yet

2 participants