Allow scalars option for SchemaBuilder

[Liam-Tait]

Instead of adding scalars after creating a schema builder using builder.addScalarType, I would like to be able to give SchemaBuilder an object with scalars.

This would set up like the following

const builder = new SchemaBuilder({
  scalars: {
    'ScalarName': Resolver
  }
})

This would help with this issue on using graphql-scalars and this discussion for custom scalar methods

As the resolvers are set up, methods can then be added for convenience.

This would look like the following with graphql-scalars and method generation. I have used the SimpleObjectsPlugin for simplicity

import SchemaBuilder from "@pothos/core"
import { EmailAddressResolver, DateTimeResolver } from "graphql-scalars"

const builder = new SchemaBuilder({
  plugins: [SimpleObjectsPlugin],
  scalars: {
    'DateTime': DateTimeResolver,
    'Email': EmailAddressResolver
  }
})

builder.simpleObject('Appointment', {
  fields: (t) => ({
    email: t.email()
    date: t.datetime(),
  })
})

This is useful as the types can be inferred from the resolver for Input and Output. This is also useful for not adding the extra methods as the field would still be correctly typed
e.g

builder.simpleObject('Appointment', {
  fields: (t) => ({
    email: t.field({type: 'Email'})
    date: t.field({type: 'DateTime'}),
  })
})

[hayes]

While this might be technically possible, there are a couple of issues:

• Input and Output types can't actually be inferred because the GraphQL classes don't actually support the right generics to make this work
• Dynamically inferring builder methods based on options gets really complicated and would require a major redesign of how options are types (breaking change) and would make the whole builder type a lot more complex, and would likely break the ability to create helper methods that accept builder instances

[Liam-Tait]

Input and Output types can't actually be inferred because the GraphQL classes don't actually support the right generics to make this work

My understanding is these would match TInternal and TExternal from GraphQLScalarType. Is that correct?

Dynamically inferring builder methods based on options gets really complicated and would require a major redesign of how options are types (breaking change) and would make the whole builder type a lot more complex, and would likely break the ability to create helper methods that accept builder instances

Makes sense. I saw this as a happy side effect made possible by having the types up front anyway.

[hayes]

My understanding is these would match TInternal and TExternal from GraphQLScalarType. Is that correct?

Interesting, not sure if I just misremembered, or this changed recently, but that would be roughly equivalent. It does look like these types are unused by all the scalars in the graphql-scalars package though.

Makes sense. I saw this as a happy side effect made possible by having the types up front anyway.

I think those types of helpers are pretty important. For this kind of major change, there would need to be a fairly compelling case for why it's needed.

Maybe a simpler approach would be a create builderWithScalars method? This could do the same thing but wouldn't really be a breaking change, it would just take all the same options as the constructor, but would return a builder with the scalar methods.

[Liam-Tait]

It does look like these types are unused by all the scalars in the graphql-scalars package though.

Hopefully these will be made public with this PR

If merged the types would be accessible.

For example: DateTimeResolver will be GraphQLScalarType<Date, string>

These types could then be extracted from the graphql-scalar and merged into the Input & Output

This can be done in user-land now

import SchemaBuilder from "@pothos/core"
import { GraphQLScalarType } from "graphql"
import { DateTimeResolver as og } from "graphql-scalars"
import { toPairs } from "remeda"

// Pretend the pr is merged and TInternal and TExternal are exposed
const DateTimeResolver = og as GraphQLScalarType<Date, string>

const scalars = { 'DateTime': DateTimeResolver } satisfies Record<string, GraphQLScalarType>

// Extract the  TInternal and TExternal types from GraphQLScalarType, map them to Input and Output for SchemaBuilder
type ExtractIO<Type> = Type extends GraphQLScalarType<infer TInternal, infer TExternal> ? {
  Input: TInternal
  Output: TExternal
} : never

// Create the type for SchemaBuilder
// for this example { 'DateTime': { Input: Date, Output: string } }
type Scalars = { [Property in keyof typeof scalars]: ExtractIO<typeof scalars[Property]> }

const builder = new SchemaBuilder<{ Scalars: Scalars }>({})

// Add the scalar types, (using remeda for type safety, could use Object.entries if preferred)
for (const [name, resolver] of toPairs.strict(scalars)) {
  builder.addScalarType(name, resolver, {})
}

builder.queryType()
builder.mutationType()

export { builder }

I've ignored the options here for brevity.

If this was to move inside of SchemaBuilder, something like ExtractIO is how I would imagine the scalars would auto-infer the types and then merge with the default types.

Maybe more realistically

const builder = new SchemaBuilder({
  scalars: [
    ['Name', resolver, options]
  ]
})

I think those types of helpers are pretty important. For this kind of major change, there would need to be a fairly compelling case for why it's needed.

The dynamic helpers feature is not required to allow scalars as a SchemaBuilder option. Being able to drop in scalars and have the types work with only t.field({type: 'Name'}) is still a win IMO. Happy to come back to dynamic helpers after figuring out scalars as a SchemaBuilder option?

[hayes]

Whoops, I think we are talking about different things with the dynamic helpers. I was talking about any 3rd party helpers depending on specific shapes of the builder, but probably out of scope for this conversation anyway.

I looked at this a bit more, and remembered the real reason I haven't done something like this (more generally inferring things from options). There are several features that would really benefit from that (eg changing default nullability).

The reason this does not work today is because typescript doesn't have a way to do partial inference. Basically we still need to be able to pass a generic to the builder constructor for other features and plugins. When you explicitly provide a generic for a function or constructor, you can't infer other values anymore.

The only way to have inference work is to use a separate method:

const builder = new SchemaBuilder<{ Context: Context }>().withScalars({
  Name: Resolver,
})

Or something similar, not sure this is actually a good API, but that is the only way you can work around the issue.

Are you just trying to get around double declaring the scalars?

The main issues I know about with the current API:

1. Custom scalars don't have helpers. eg t.date
2. You need to add the types and definition separately
3. They are not well documented, and people have found setting them up confusing

I think something like withScalars could be a big improvement, and could theoretically add the t.date style helpers as well.

This would still require a fairly large (breaking) change to how the helpers are added to field builders. I'd likely want to standardize to make all the helpers dynamically generated including the built in scalars for consistency. The list of scalars would need to be stored on the builder.

I'm currently on vacation and in my spare time working on a big 4.0 release. If you really want this feature, getting it in as part of 4.0 would be the time to do it (still a long way out, probably a couple months)

[Liam-Tait]

Whoops, I think we are talking about different things with the dynamic helpers. I was talking about any 3rd party helpers depending on specific shapes of the builder, but probably out of scope for this conversation anyway.

Totally haha. Ok, that makes sense

withScalars

That would make sense to me.
Some prior work on this pattern is fastify's withTypeProvider. I use it and it works well for extending typescript functionality.

Nice win that it is a non-breaking change too.

Are you just trying to get around double declaring the scalars?

That is pretty much it.

I wanted to use JsonObjectResolver and DateTimeResolver from graphql-scalars and I felt I needed to go into the graphql-scalars source code to see what was going on there to know which types were correct.

They are not well documented, and people have found setting them up confusing

If you point me in a direction I am happy to write some docs.
I referenced Adding graphql scalars to understand what was going on here and thought it was all ok. Figuring out which types to use was the issue for me.

I'm currently on vacation and in my spare time working on a big 4.0 release

🏖️💻 No rush at all

If you really want this feature, getting it in as part of 4.0 would be the time to do it (still a long way out, probably a couple months)

Cool, Would that be aiming the pr at the v4 branch?

Would adding withScalars require the major release?

[hayes]

Adding withScalars wouldn't require a major version, but rebuilding the field builder classes to generate the correct helpers might.

If you want to include the helpers, I'd Target v4, but it's completely broken right now and I have a bunch of changes in progress from my last flight I haven't pushed.

I think a PR to main that doesn't add the helpers would be an easier place to start and wouldn't require a major version