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

Exclude unused standard types from the schema #974

Merged
merged 7 commits into from
Oct 19, 2021

Conversation

spawnia
Copy link
Collaborator

@spawnia spawnia commented Oct 15, 2021

Improves our behaviour in regards to #964, except Schema::getType()

@spawnia
Copy link
Collaborator Author

spawnia commented Oct 15, 2021

Should Introspection::fromSchema() throw on errors?

@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #974 (cb236ae) into master (694088f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #974   +/-   ##
=========================================
  Coverage     94.27%   94.27%           
  Complexity       50       50           
=========================================
  Files           118      118           
  Lines          9577     9579    +2     
=========================================
+ Hits           9029     9031    +2     
  Misses          548      548           
Impacted Files Coverage Δ
src/Type/Introspection.php 99.69% <100.00%> (+<0.01%) ⬆️
src/Type/Schema.php 88.88% <100.00%> (+0.06%) ⬆️
src/Utils/BuildClientSchema.php 100.00% <100.00%> (ø)
src/Utils/TypeInfo.php 95.67% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 694088f...cb236ae. Read the comment docs.

@spawnia spawnia marked this pull request as ready for review October 15, 2021 09:39
Copy link
Contributor

@vhenzl vhenzl left a comment

Choose a reason for hiding this comment

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

Schema::getType() should return null for unused standard types (graphql-js returns undefined): https://github.com/graphql/graphql-js/blob/v15.6.1/src/utilities/__tests__/buildASTSchema-test.js#L161-L168

There is a test for it in 33c6bc1 (#928) you can copy here.

@spawnia
Copy link
Collaborator Author

spawnia commented Oct 18, 2021

Thanks @vhenzl for bringing that up. I noticed that difference before and changed the implementation of a test to use getTypeMap() instead.

I documented the difference for now (see #964 (comment)) and plan to leave the issue open until we can resolve it fully. Unless you have an idea how to fix our implementation, I would like to go ahead with this PR. It does at least fix the externally observable behaviour in regards to introspection.

@vhenzl
Copy link
Contributor

vhenzl commented Oct 19, 2021

Right, makes sense.

@spawnia spawnia merged commit d53d688 into master Oct 19, 2021
@spawnia spawnia deleted the exclude-unused-standard-types-from-the-schema branch October 19, 2021 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants