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

Allow extending root fields #272

Open
nhuesmann opened this issue Jan 13, 2020 · 7 comments
Open

Allow extending root fields #272

nhuesmann opened this issue Jan 13, 2020 · 7 comments
Labels
scope/schema Related to the schema component type/feat Add a new capability or enhance an existing one

Comments

@nhuesmann
Copy link

I think Nexus's mutationField/queryField create a simple, intuitive, and clean way to have a modular schema. I would love to see them made available through graphql-santa.

@jasonkuhrt jasonkuhrt added the scope/schema Related to the schema component label Jan 14, 2020
@jasonkuhrt
Copy link
Member

jasonkuhrt commented Jan 14, 2020

Thanks for raising this @nhuesmann. Educated myself a bit tonight around this topic.

SDL Way

Modularized schemas work in schema-first approach via the extend SDL keyword. Example:

type Query {
  a: String
}
extend type Query {
  b: String
}
extend type Query {
  c: String
}

Is equivalent to:

type Query {
  a: String
  b: String
  c: String
}

Nexus Way

Nexus models extend with extendType and has convenience functions mutationField and queryField.

queryType({
	definition(t){
		t.string('a')
	}
})

queryField("b", {
	type: 'String',
	resolve: () => 'foo',
})

queryField("c", {
	type: 'String',
	resolve: () => 'foo',
})

Is equivalent to:

queryType({
	definition(t){
		t.string('a', () => 'foo')
		t.string('b', () => 'foo')
		t.string('c', () => 'foo')
	}
})

Santa Way (proposal)

I'm thinking that santa should special case root types (Query Subscription and Mutation) such that multiple declarations of them merge automatically.

queryType({
	definition(t){
		t.string('a')
	}
})
queryType({
	definition(t){
		t.string('b')
	}
})
queryType({
	definition(t){
		t.string('c')
	}
})

Would be equivalent to:

queryType({
	definition(t){
		t.string('a', () => 'foo')
		t.string('b', () => 'foo')
		t.string('c', () => 'foo')
	}
})

I think this is a good direction because:

  • It looks like the simplest possible API for the user

  • Arguably this API is more declarative in regards to modelling the idea of distributed root types. Here that idea––distributed root types––is first class, while with extend approach its not, its achieved by saying how (extend, extend, extend, ...)

  • Root types are special already, specified in the spec

    • hence why Nexus has queryType shorthand to begin with
    • hence why Nexus can default to schema of Query { ok: true }

    Point being: Its not unnatural to imbue them with this special behaviour

  • Problem: In SDL, having to define Query once before being able to extend it later is just not helpful or what I think most people want most of the time; In other words why force the user to keep track of/manage "is this my first instance or nth instance of e.g. Query.

    With this system, user doesn't have to care

  • Problem: In Nexus, the shorthands mutationField and queryField expand the API surface area. It is a common use-case to modularize the root type fields. Ideally the API for it should be nothing new to learn.

    With this system, no new APIs to learn

  • With this system, the API scales more gracefully: user can grow their project from one schema module to many without having to think about changing how they manage root types.

No obvious reason comes to mind why this proposed merging behaviour might be undesirable.

@jasonkuhrt jasonkuhrt added type/feat Add a new capability or enhance an existing one impact/high labels Jan 14, 2020
@jasonkuhrt jasonkuhrt pinned this issue Jan 14, 2020
@jasonkuhrt jasonkuhrt changed the title Expose Nexus's mutationField and queryField methods Allow extending root fields Jan 14, 2020
@jasonkuhrt
Copy link
Member

@nhuesmann FYI I'm rephrasing the title of this issue to be able the problem rather than solution.

@nhuesmann
Copy link
Author

@jasonkuhrt I love it, I agree with all your points. Changing queryType to be extensible rather than adding a new API to learn is an excellent solve. Like you said, the ability to start with a basic example and grow/modularize it without having to learn anything new is an excellent DX.

@kevinwolfcr
Copy link
Contributor

Excellent input, @jasonkuhrt. Just wondering, since santa uses nexus under the hood, doesn't it need to first implement this behavior, so later santa just upgrades the dependency of nexus with no further configuration?

@Weakky
Copy link
Collaborator

Weakky commented Jan 14, 2020

We indeed forgot to expose some part of the nexus surface api, I started a draft here #276 (exposing the rest of the nexus api without your proposal yet @jasonkuhrt)

Regarding your proposal, there is a problem:

Right now, what we could do is use extendType for queryType and mutationType (extendType works even when there's no type to extend. It just create an empty object type in that case). But we'll loose some configuration input. extendType does not accept the following options:

image

Reason being that you should define these configurations on the main queryType or objectType({ name: 'Query', ... }) (or mutationType, doesn't matter), and you shouldn't be able to define them twice.

eg:

app.queryType({
  definition(t) {...},
  description: "something",
  nonNullDefaults: { output: true },
})

app.queryType({
  definition(t) {...},
  description: "something else",
  nonNullDefaults: { output: false },
})

We could probably have an internal nexus plugin to warn if multiple root types were defined with different configurations, but it still makes the API error prone.

Unless we find a better solution, I'd personally stick to the nexus API. WDYT?

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Jan 14, 2020

Some ideas;

  1. The configurations can stay local to their instance of the root type def. Except for description which we can either remove or treat as a merge, doing string appends. Maybe a newline between each ala paragraphs.

  2. Or we could simplify the root type def blocks and allow another central place to configure it.

Not sure yet, will think more about it.

I like 1 so far.

@Weakky
Copy link
Collaborator

Weakky commented Apr 21, 2020

Dug into this yesterday and this morning.

The configurations can stay local to their instance of the root type def. Except for description which we can either remove or treat as a merge, doing string appends. Maybe a newline between each ala paragraphs.

By moving the type config to each instance of query/mutationType, there's no way anymore to have a single config for the type (except by using objectType()) though.

Furthermore, keeping type configuration local to each instance of the root type is not an inherently complex task but requires a substantial refactor on @nexus/schema. I identified two concrete tasks to do, at different level of complexity IMO:

  1. Typegen (easy one)

@nexus/schema takes a GraphQLSchema as input into typegen, and stores nexus relevant information on the schema itself via the extensions property. This additional information attached to the schema contains stuff like the nonNullDefaults to properly type the schema. Right now, this information is stored for the whole type.

By the time we get into typegen, we loose all the context about which type config is associated with which field. We need to refactor nexus to keep the type configuration associated with every field of the type

  1. Plugins

Plugins can add configuration properties to object types. Thanks to the plugin({ objectTypeDefTypes }) option. This mean:

  • Plugin configuration needs to be available on queryType and mutationType (and therefore to extendType)
  • Plugin configuration + execution needs to stay local to each instance of extendType
  • Just like for nonNullDefaults, there would be no way anymore to have a single plugin configuration for the whole type. eg: authorize
  • extendType can be used to extend both output types and input types. But plugin configuration can only be added to object types. If we add plugin configuration to extendType, it creates conflicts because plugin configuration should only appear if you're extending an object type and not if you're extending any other type

Note: We might have less problems if we take the opinionated approach of making root types special (Query/Mutation/Subscription) and not expending the concept to any output types. I did not investigated that approach, but it will probably require a lot of refactor too as @nexus/schema is very little opinionated in regards to root types (besides adding a Query type if your schema is empty)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope/schema Related to the schema component type/feat Add a new capability or enhance an existing one
Projects
None yet
Development

No branches or pull requests

4 participants