[WIP] graphql schema refactor #10995
Conversation
There was a problem hiding this comment.
After a quick look. I'm not trying to nitpick on changes, but I really would want to try to make this PR as small as possible and to change as little things is possible, because then we can:
- Test in efficiently
- Minimize api surface changes to the users
- Do other good changes in subsequent PRs.
I'm really happy to help with that, but it's a massive change, so I'd be really helpful if you briefly explain some of the changes.
Thanks!
| @@ -1,90 +0,0 @@ | |||
| const _ = require(`lodash`) | |||
There was a problem hiding this comment.
Could you explain the change to node-tracking?
| @@ -170,3 +170,65 @@ emitter.on(`DELETE_PAGE`, action => { | |||
| const node = getNode(nodeId) | |||
There was a problem hiding this comment.
Do we need to add typeDefs to all Gatsby plugins? Which ones do we add this to? Can we do it in separate PRs while keeping basic functionality working?
There was a problem hiding this comment.
While not necessary I think it's a good idea to add typedefs to internal plugins, since there's no need to infer those types on every run. Also, we should rethink the status of the File type, which we currently treat as if it were internal (e.g. we auto-infer File fields), but rely on a not-internal plugin to handle.
| @@ -1,97 +0,0 @@ | |||
| const runSift = require(`../run-sift`) | |||
There was a problem hiding this comment.
Were those tests moved somewhere?
There was a problem hiding this comment.
removed, but there are tests in schema/query/sift
| @@ -2,55 +2,44 @@ const Redux = require(`redux`) | |||
| const _ = require(`lodash`) | |||
| const fs = require(`fs`) | |||
| const mitt = require(`mitt`) | |||
| const stringify = require(`json-stringify-safe`) | |||
| const v8 = require(`v8`) | |||
There was a problem hiding this comment.
Could you summarize the changes here and why they were required?
There was a problem hiding this comment.
This is indeed unrelated, see PR #10732
There was a problem hiding this comment.
There are other changes in get-nodes-for-query that are perf-related and could easily be extracted: (i) also cache during bootstrap, not only in production, and (ii) first check if there actually are resolvers to manually call, before iterating over all nodes.
There was a problem hiding this comment.
And use a separate redux namespace for nodesByType.
There was a problem hiding this comment.
Awesome, would be great if you could extract those changes, I think they are useful outside of the context of this pr :)
| @@ -0,0 +1,179 @@ | |||
| const { TypeComposer, schemaComposer } = require(`graphql-compose`) | |||
| const { SchemaDirectiveVisitor } = require(`graphql-tools`) | |||
There was a problem hiding this comment.
[minor] I'd rather test such things by running GraphQL queries, rather than by calling resolvers.
| @@ -0,0 +1,19 @@ | |||
| const { | |||
There was a problem hiding this comment.
Let's expose this to the user in some way, as discussed before in the issue
There was a problem hiding this comment.
Should be on context, see query-runner
| findOne, | ||
| paginate, | ||
| } = require(`./resolvers`) | ||
| const link = require(`./link`) |
There was a problem hiding this comment.
Maybe rename to "getByField"?
There was a problem hiding this comment.
Sure, sounds good
|
|
||
| // FIXME: Handle array of arrays | ||
| // Maybe TODO: should we check fieldValue *and* info.returnType? | ||
| const link = ({ by }) => (source, args, context, info) => { |
There was a problem hiding this comment.
Feels a bit magical as a general resolver, what about eg Unions or Interfaces? I don't mind the directive though.
There was a problem hiding this comment.
Agreed, Unions need support from graphql-compose though
|
@freiksenet Thanks for your comments! I think I should start by saying that I have been working on this branch as a hobby learning project, and literally the first thing I did was delete everything in the schema/ folder and start from scratch. So it's probably best to not base the schema refactor off of this, but view it more as a working prototype to take some ideas from, especially if we want to keep this as minimally invasive as possible. As for the rest, comments inline. |
|
@stefanprobst Sounds good :) I think the minimal PR would have graphql-compose backed schema, addTypeDefs and addResolvers functions and maybe the model layer. I probably won't have much time to work on it next week, but I can start or help with it after that. Looking forward to the call! |
See #4261