Don't parse the GraphQL schema twice on load

[Vultraz]

Apollo already accepts the AST output of gql. No need to parse the schema both in ks and then again in Apollo.

Honestly, I might make dumpSchema just return a string so we can avoid that boilerplate in the tests. 😐

[changeset-bot]

🦋 Changeset is good to go

Latest commit: bcd1926

We got this.

This PR includes changesets to release 18 packages

Name	Type
@keystonejs/keystone	Major
@keystonejs/adapter-knex	Patch
@keystonejs/adapter-mongoose	Patch
@keystonejs/test-utils	Patch
@keystonejs/demo-project-blog	Patch
@keystonejs/demo-custom-fields	Patch
@keystonejs/demo-project-meetup	Patch
@keystonejs/demo-project-todo	Patch
@keystonejs/cypress-project-access-control	Patch
@keystonejs/cypress-project-basic	Patch
@keystonejs/cypress-project-client-validation	Patch
@keystonejs/cypress-project-login	Patch
@keystonejs/cypress-project-social-login	Patch
@keystonejs/benchmarks	Patch
@keystonejs/example-projects-blank	Patch
@keystonejs/example-projects-nuxt	Patch
@keystonejs/example-projects-starter	Patch
@keystonejs/example-projects-todo	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[timleslie]

Honestly, I might make dumpSchema just return a string so we can avoid that boilerplate in the tests. 😐

Yeah, this is a slightly awkward corner of the keystone object API. getTypeDefs is good when you're Apollo, and dumpSchema is good if you want to export your schema to a file, but we don't have anything in between for when you want your schema returned as just a string.

This PR should really be a major change, since we're changing the return type of getTypeDefs. If we're going to do that then I think we should really resolve the whole API all at once.

I think we could easily have dumpSchema return schema. If we also put in a check for if (file) and default the value of schemaName to "public" then we could have a backwards compatible API that would let you do:

console.log(keystone.dumpSchema())

and have it do something meaningful.

If we wanted to go with a backwards incompatible change then I'd propose the same behaviour but just change the methods args to be an object, e.g. dumpSchema({ file, schemaName = 'public' }).

Thoughts?

[MadeByMike]

@timleslie This looks good to me. What do you think?

(EDIT: Async error, left review tab open while Tim was commenting)

[Vultraz]

Idea I had last night want to make dumpSchema just return a string, then add a --dumpSchemaTo (or similarly named) argument to the CLI that handles the write-to-file bit (and of course, anyone else could just write to file manually, since it's just one line).

Right now, the only place dumpSchema is actually mentioned is the performance monitoring docs. They're pretty outdated at this point (working on that), but the method they recommend to use dumpSchema is awkward.

[timleslie]

I think I'd be in favour of your suggestion of making dumpSchema simply return a string. It could then be: dumpSchema(schemaName = 'public') {..., and people could then do whatever they wanted with that string, whether it's writing it to a file, dumping it to the console, etc, etc.

I don't think there's any value in adding a CLI option, it just feels like API cruft to me.

[Vultraz]

@timleslie ok, done

[Vultraz]

BTW, does this really need to be a major patch? getTypeDefs is an internal function and dumpSchema is basically undocumented. I don't think it make sense to do a major version bump any time any API changes.

[timleslie]

👍