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

V8. Custom scalars types generation #1228

Closed
ziimakc opened this issue Oct 26, 2024 · 22 comments · Fixed by #1242 or #1244
Closed

V8. Custom scalars types generation #1228

ziimakc opened this issue Oct 26, 2024 · 22 comments · Fixed by #1242 or #1244

Comments

@ziimakc
Copy link

ziimakc commented Oct 26, 2024

Perceived Problem

Currently it's unclear on how to provide types and parsers for custom gql scalars for generation step. There is a docs for client which resolve it partly, but I would question why it present there instead in generation step.

import { Generator } from "graffle/generator";
import { join } from "node:path";

await Generator.generate({
	schema: {
		type: "sdl",
		dirOrFilePath: join(
			import.meta.dirname,
			"../../api/generated/schema.gql",
		),
	},
	outputDirPath: join(
		import.meta.dirname,
		"../node_modules/generated/graffle",
	),
	name: "GraffleClient",
});

Ideas / Proposed Solution(s)

...

await Generator.generate({
	customScalars: [
		{
			key: "Date",
			decode: (value) => new Date(value),
			encode: (value) => value.toISOString(),
		},
	],
});

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Oct 26, 2024

So originally it was ONLY possible via generation, however I decided that requiring gentime config was too complex and instead wanted to let them be used in a regular runtime way.

The only advantage of gentime is some static APIs that can be exported as a result.

I'll finish the docs for this soon!

@ziimakc
Copy link
Author

ziimakc commented Oct 26, 2024

@jasonkuhrt but how then query types will be resolved?

Let's say I do:

	const gql = await GraffleBuilder.create({
		transport: { methodMode: "post" },
		schema: new URL("http://localhost:3001/api/graphql"),
	});

        // {userId: string, createdAt: string }
	const data = await gql.query.user({
		userId: true,
		createdAt: true,
		$: { userId: "" },
	});

I assume currently it falls back to string type while I have UUID scalar, how would I redefine it if not in generation step?

Gql-codegen does it like this:

const config: CodegenConfig = {
  schema: '**/schema.graphql',
  generates: {
    'src/schema/types.generated.ts': {
      plugins: ['typescript'],
      config: {
        scalars: {
          // Recommended ID scalar type for clients:
          ID: {
            input: 'string | number',
            output: 'string'
          },
          // Setting custom scalar type:
          CustomScalar: {
            input: 'string', // this means our server can take CustomScalar as string
            output: 'number' // this means our server will return CustomScalar as number
          }
        }
      }
    }
  }
}

@jasonkuhrt
Copy link
Member

Did you check out the customer scalar example?

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Oct 26, 2024

One thing not well explained yet is that there is a SDDM (Schema Driven Data Map) that must be given to the Graffle instance for custom scalars to work. When importing the generated client it is ready provided. You can see this for yourself by opening the client module in the generated code.

@ziimakc
Copy link
Author

ziimakc commented Oct 26, 2024

@jasonkuhrt hm, while argument of type UUID changed to Date, returned userId is still string

scalar UUID

type User {
	userId: UUID!
}
	const gql = await GraffleBuilder.create({
		transport: { methodMode: "post" },
		schema: new URL("http://localhost:3001/api/graphql"),
	}).scalar("UUID", {
		decode: (value) => new Date(value),
		encode: (value) => value.toISOString(),
	});

          // still string
         // {userId: string, createdAt: string }
	const data = await gql.query.user({
		userId: true,
		createdAt: true,
                // now it's a date
		$: { userId: new Date()  },
	});

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Oct 27, 2024

That's because you haven't applied the SDDM. You need to pass the schema map to the constructer.

I am planning to make .scalar a never like type when the SDDM is missing which will help make this more clear.

@ziimakc
Copy link
Author

ziimakc commented Oct 27, 2024

@jasonkuhrt Ok, I think I found a correct import, now it works:

import { Schema } from "graffle/schema";
Schema.Scalar.create(...)

But it's a bit confusing, in schema userId in argument and query is UUID scalar, why do I need set scalar in two different places, one for query return and one for arguments.

import { Schema } from "graffle/schema";

// this applies only to `User` query type
export const UUID = Schema.Scalar.create("UUID", {
	encode: (value: Date): string => value as any,
	decode: (value: string): Date => value as any,
});


	const gql = await GraffleBuilder.create({
		schema: new URL("http://localhost:3001/api/graphql"),
         // this applies to arguments
	}).scalar("UUID", {
		decode: (value) => new Date(value),
		encode: (value) => value.toISOString(),
	});

And why do I need to do it at client creation side if all types are known at build time, this part should be just moved to generator config replacing hard to use customScalarCodecs:

.scalar("UUID", {
		decode: (value) => new Date(value),
		encode: (value) => value.toISOString(),
	});

@ziimakc ziimakc changed the title V8. Custom scalars types generation docs V8. Custom scalars types generation Oct 27, 2024
@jasonkuhrt
Copy link
Member

jasonkuhrt commented Oct 27, 2024

But it's a bit confusing, in schema userId in argument and query is UUID scalar, why do I need set scalar in two different places, one for query return and one for arguments.

You should only have to set it once. This should be enough:

	const graffle = Graffle
      .create({
		schema: new URL("http://localhost:3001/api/graphql"),
	  })
      .scalar("UUID", {
		decode: (value) => new Date(value),
		encode: (value) => value.toISOString(),
	  })

And why do I need to do it at client creation side

Custom Scalars is actually a runtime and buildtime concern.

  1. The encode and decode steps happen at runtime.
  2. The input and output TS types are builditme.

We can infer types from terms but not the other way around.

As mentioned the only advantage of the generator having access to the scalar types is to do smarter things with exports. For example there is a static select utility for building up custom selection sets. In order for that to have the correct types one of two things need to happen:

  1. Gentime has access to the custom scalars
  2. The utility type is coming from the Graffle chain e.g. graffle.scalar(One).scalar(Two).select(...).

@ziimakc
Copy link
Author

ziimakc commented Oct 27, 2024

You should only have to set it once. This should be enough:

	const graffle = Graffle
      .create({
		schema: new URL("http://localhost:3001/api/graphql"),
	  })
      .scalar("UUID", {
		decode: (value) => new Date(value),
		encode: (value) => value.toISOString(),
	  })

This only changes types for gql query arguments, not for gql type, so if I do:

// user.userId will be string and I need to `customScalarCodecs` for generator to change that.
const user = await gql.query.user();

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Oct 27, 2024

@ziimakc I shipped an improvement wherein the custom scalars will be profiled in the generated constructor for convenience. I have also started documentation of custom scalars here: https://graffle.js.org/guides/appendix/custom-scalars. This should chip away at some of the friction you experienced. Let me know.

This only changes types for gql query arguments, not for gql type, so if I do:

Thanks for explaining, I think I saw something similar today while working, it's a bug, I'll look into this next.

@jasonkuhrt
Copy link
Member

Made some double checks and everything is working as expected #1232 with regards to types. Could you share more about your issue so I can reproduce it?

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Oct 28, 2024

Regarding your idea about a generator config, there are two issues there:

  1. We'd have to use the typescript compiler or like to extract that code and emit it somewhere. While this isn't so uncommon nowadays (e.g. nextjs/remix/etc. doing code splitting between backend and front end code) it's pretty complex for me to take on right now.
  2. It wouldn't be practical to pass that over the CLI.

So the current design aims for the simplest next option.

I really don't like the approach of gql codegen because there is no connection between buildtike and runtime. It's cheating via strings that represent types.

@jasonkuhrt
Copy link
Member

Hey @ziimakc, just friendly ping if you have a moment to and want to let me know about your issue.

@ziimakc
Copy link
Author

ziimakc commented Oct 31, 2024

@jasonkuhrt here is a schema:

scalar UUID

type User {
	userId: UUID!
}

type Query {
	user(userId: UUID!): User!
}

Setting this changes only argument type:

	const graffle = Graffle
      .create({
		schema: new URL("http://localhost:3001/api/graphql"),
	  })
      .scalar("UUID", {
		decode: (value) => new Date(value),
		encode: (value) => value.toISOString(),
	  })

So it becomes something like this in typescript types:

type User {
	userId: UUID! <- this can be changed only be generator custom scalar file
}

type Query {
	user(userId: Date!): User!
}

@jasonkuhrt
Copy link
Member

Thanks, I'll try to make another repro. Still not sure what's going on.

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Oct 31, 2024

fwiw I just shipped #1241 which helps reduce user error around this topic.

CleanShot 2024-10-31 at 09 40 43

@jasonkuhrt
Copy link
Member

@ziimakc in your example you've mistakenly registered a Date scalar. It will never match up with your UUID. Can you fix that on your side and report back?

Meanwhile, I am going to ship another improvement that limits the registered scalars to those that appear in the schema for exactly this kind of reason.

@jasonkuhrt
Copy link
Member

Hey, a lot of improvements have been shipped since this issue was opened. I just shipped a last one that addresses better catching the error in your example. I'll consider this closed but new issues for any further problems are always welcome. Thanks for the batch of feedback you provided over the past week it was awesome to have that. Thank you!

@ziimakc
Copy link
Author

ziimakc commented Oct 31, 2024

@jasonkuhrt > in your example you've mistakenly registered a Date scalar
this was not mistake, but example, this is how you change typescript type of UUID argument.

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Oct 31, 2024

@ziimakc Ah sorry I thought you'd written .scalar("Date". I'll check again :)

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Oct 31, 2024

@ziimakc so sorry, finally found the issue. Check the PR, one liner, "funny"... I was soooo confused and there was test coverage. If I had of tested two scalars this issue would have become apparent but it appeared to be working only because the tests were always using Date scalars.

I appreciate your patience and raising this issue repeatedly. I'm sure you saved someone(s) else's time in the future 😀.

I use Cursor editor and it often autocompletes stuff for me. There is a good chance this came from that haha.

@jasonkuhrt
Copy link
Member

@ziimakc hey just noticed you had set a calendar meeting together. I see it's canceled now but happy to chat if you change your mind. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants