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

Avoid eager-loading schema unnecessarily #1104

Merged
merged 39 commits into from
May 24, 2022
Merged

Conversation

spawnia
Copy link
Collaborator

@spawnia spawnia commented Apr 7, 2022

Resolves #1093
Resolves #954
Resolves #1137

vhenzl and others added 9 commits September 4, 2021 21:12
- Fixes asserts and indentation
- Preserves additional tests and changes for repeatable and interfaces implementing interfaces.
- Doesn't add these two missing tests as they tests unsupported lagacy names:
  - it('maintains configuration of the original schema object'
  - it('adds to the configuration of the original schema object'
Tests no longer present in `graphql-js` has been moved to `SchemaExtenderLegacyTest`.
Respective commits that removed them from `extendSchema-test.js` are mentioned in comments.

Tests fail because:
- `repetable` in directives is lost when calling `extend()`
- `Schema::getAstNode()` returns `null` for an extended schema
- scalar types are added to schema automatically even if they are not used in SDL
Reasons, why tests fail (in addition to those mentioned in the previous commit):
- a problem with parsing/printing of types without fields (e.g. `type Query`)
- string comments become multiline (e.g. `"""Comment"""` becomes `"""\nComment\n"""`
- different order of definitions affects `printSchemaChanges()`
- missing `@` in directive name in error messages
# Conflicts:
#	src/Utils/SchemaExtender.php
@spawnia
Copy link
Collaborator Author

spawnia commented May 5, 2022

@vhenzl do you have an idea why the extended schema order differs from graphql-js and how we can fix it?

@vhenzl
Copy link
Contributor

vhenzl commented May 5, 2022

@spawnia, not sure what you mean. Different order of types in a printed schema? It would be #954.

@spawnia
Copy link
Collaborator Author

spawnia commented May 5, 2022

Some test cases are failing due to the order of the printed output, see https://github.com/webonyx/graphql-php/runs/6302953843?check_suite_focus=true

I went ahead and removed sorting from the SchemaPrinter, now some tests fail due to the changed output which should be trivial to fix.

@spawnia spawnia marked this pull request as ready for review May 6, 2022 09:21
@spawnia
Copy link
Collaborator Author

spawnia commented May 11, 2022

@vhenzl can you review this?

@spawnia spawnia merged commit 9791dc1 into master May 24, 2022
@spawnia spawnia deleted the avoid-eager-loading-schema branch May 24, 2022 15:41
@spawnia spawnia mentioned this pull request May 24, 2022
@ruudk
Copy link
Contributor

ruudk commented May 24, 2022

@spawnia After using dev-master and dumping the schema, the types are no longer sorted A-Z.

Why are the types no longer sorted? This is super useful for diffing the persisted schema that is stored in git. We do that for example so that other projects can also pull in the schema.

When the types are now random or based on how they are discovered, it's gonna trigger more changes from time to time without no reason.

@spawnia
Copy link
Collaborator Author

spawnia commented May 24, 2022

The definition order of the schema is now kept. This is how the reference implementation does it now. We generally follow them, it makes maintenance much easier.

@ruudk
Copy link
Contributor

ruudk commented May 24, 2022

What if we bring this back as an option to the SchemaPrinter?

SchemaPrinter::doPrint($schema, ['sort' => true])

It will then run this again:

$types = $schema->getTypeMap();
ksort($types);
$types = array_filter($types, $typeFilter);

@ruudk
Copy link
Contributor

ruudk commented May 25, 2022

@spawnia Brought back the sorting of types as an option to the SchemaPrinter: #1152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants