-
Notifications
You must be signed in to change notification settings - Fork 61
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
WIP: GraphQL 4 #266
WIP: GraphQL 4 #266
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slooowly making my way through this epic journey. I'll paste feedback in increments to avoid losing them to the browser gods at some point.
In light of this, it makes sense to give these tools their own namespace in dev/graphql.
That's not actually in the PR yet, right? Maybe start a public todo list to make it easier to review?
Here's how to get this going for local testing (on
Then run a |
Required for dev/tasks/schema-build which will loop over multiple schemas by default.
Subclassing DevelopmentAdmin is only okay for a PoC IMO, unless we can guarantee that this is the only module in existence that subclasses it. Otherwise any other module that subclasses it will either
Adding an additional extensibility feature to DevelopmentAdmin is probably a better approach EDIT: oops, it's not an injector replacement so my biggest concern goes away. |
I'm testing out a new composer install of 4.6 with this module setup and just noting down some things as I go:
|
Ironic error message |
Yup, all the modules are currently "render only". Long way to go, but the work is actually really straightforward in what I've seen so far. It's like a hackday or two. |
Thank you for tracking that, though. One less thing to test when we finish the asset admin upgrade. 😁 |
Talked to Aaron, keeping this PR open is starting to obstruct collaboration and further review. I think we've taken it far enough to prove that the API as it is designed has legs, and that the core premise (make it faster) is satisfied. So while the tests are failing, and there's still lots of work left, this can happen as follow on work in the |
Thank you for making my sites faster. Hope it works in practice! |
GraphQL v4: Let's scale this thing
Note: This document will continue to be updated as changes evolve. It is not a comprehensive view of everything that will be included in v4, but rather an overview of where we want to be and how things are going.
This pull request represents a large body of changes to the GraphQL module that enhance its performance and developer experience on a very fundamental level. It is a complete rewrite of the entire schema-facing APIs including scaffolding, type creators, middleware, and more.
High-level goals/changes
This section describes the overarching intention of the changes. For information on upgrading, see the "Upgrading" section below.
Drastically improved performance
Some rough testing showed plenty of merit to the chosen approach:
While there appears to be some variance in the results, it may very well be possible that the response times do not scale with the number of types queries -- in fact, due to the new lazy loading approach, we would expect this to be the case. That the response times seem to increase with types could just be noise in the averaging of response times that varied quite a bit.
tl;dr this needs more testing, but we know we're on the right path.
No more dynamic schema generation
Generating the schema at runtime was the biggest performance bottleneck for this module and the primary reason why it could not be scaled out for heavy production use. This could not go on.
Instead of a dynamically generated schema, we build one in a new task, e.g.
/dev/graphql/build
with code generation. This will be a major change to the developer experience, but we believe the tradeoffs are worth it. It means that any additions of fields or changes to types, resolvers, etc., will require a separate build. It's possible that we could bundle this withdev/build
, however it should be noted that while the two tasks are often interdependent, there are many cases where you need one and not the other. At the moment, the build schema task appears to be many times faster thandev/build
.Manager has been removed broken up into multiple concerns
The
Manager
class was becoming a catch-all that handled registration of types, execution of scaffolding, running queries/ middleware, error handling, and more. This has been broken up into a few new classes, primarily:SchemaBuilder
<-- the new config namespace for your schemaQueryHandlerInterface
3: Much more generous configuration API
In GraphQL 4, we'll aim to build schemas with config by default, and code will fill in the gaps for parts of the schema that can only be computed at build time. A thorough review of the current "TypeCreator" APIs, with the exception of their resolvers, are just value objects that map to
graphql-php
primitives. This can be done declaratively.4: Resolvers use a new discovery pattern
Types can be created declaratively, but what about resolvers? The key to caching the schema has always been moving to static resolvers rather than context-aware class members. To solve this problem, we'll use a
ResolverRegistry
class where the developer can register any number ofResolverProvider
implementations. Those implementations must provide logic on how to locate the static method given a type name and field name. Below is a simple resolver provider that ships with the module:A type named
Product
with a field namedPrice
will look in the resolver registry in this order:resolveProductPrice
resolveProduct
resolvePrice
resolve
An important note here is that this computation is not happening at query time. It solely exists to determine what static method will be added to the generated schema PHP code.
Non-dataobject types are just... config
With resolvers relying on their own discovery logic, writing new types becomes exceedingly simple:
Notice the ability to inline arguments.
Goodbye, scaffolders, hello models
This version decouples DataObjects from the module and abstracts them away into a concept called "models." A model is any class that can express awareness of its schema. It should be able to answer questions like:
By default, we ship a DataObject implementation of a model. The configuration looks like this:
Note that dataobjects are no longer one-to-one with their types. You can have one dataobject map to multiple types.
Resolver contexts
For scaffolded operations, context is critical to the resolvers. They need to know what class to get in the ORM, for instance. For this, you can use resolver context. Fields accept resolver context and pass it as the argument to the resolver function.
Enums are first-class citizens
You can now define enums as plain config:
Lowercase field names are encouraged and supported
The current pattern of mapping field names to their
UpperCase
counterparts in the ORM is a bit unconventional. If you express them in lowercase, they'll just work.New GraphQL developer tools
The developer API for GraphQL has expanded quite a bit in the lifetime of this module, and with these new changes, we'll now have three dev tools all with their own endpoints:
In light of this, it makes sense to give these tools their own namespace in
dev/graphql
. We'll have:dev/graphql/ide
dev/graphql/schema
dev/graphql/build
dev/graphql/resolvers
New performance middleware
Since the primary focus of this version is performance improvements, a performance middleware will ship by default, enabled in non-live modes:
Upgrade to graphql-php ^14.0
The new lazy loading of types makes this all possible.
Upgrading
Again, this is a work in progress, and should mostly be considered a live document that's getting updated as APIs change:
SilverStripe\GraphQL\Manager
should be migrated toSilverStripe\GraphQL\Schema\SchemaBuilder
SilverStripe\GraphQL\Controller
implementations should be passed aSchemaBuilder
instance rather thanManager
.scaffolding.types
config should be moved tomodels
scaffolding.queries
andscaffolding.mutations
should be moved toqueries
andmutations
at the top level ofSchemaBuilder
config.ResolverProvider
implementation. Recommend subclassing the abstractDefaultResolverProvider
class and writingresolve{TypeName}{FieldName}
methods as described above.*Creator
classes should be migrated to config. The resolver should be made static and moved into aResolverProvider
implementation.Installing the WIP
Use our test project.
Or extract the relevant bits from
composer.json
into your own project.Then run a
sake dev/build
, followed by asake dev/tasks/build-schema
.Read the draft docs at https://pulls-master-schemageddon--ss-docs.netlify.app/en/4/developer_guides/graphql/
See also
Remaining todos:
Critical for merge
Nice to haves, post merge
Compatibility