Validate that all context types match #29

captbaritone · 2023-11-03T15:37:43Z

Fixes #21. Directly implements what @mandx suggested in #21 (comment).

An alternative /** @gqlContext */ approach is explored in #24.

netlify · 2023-11-03T15:37:51Z

✅ Deploy Preview for grats ready!

Name	Link
🔨 Latest commit	`8ee1d65`
🔍 Latest deploy log	https://app.netlify.com/sites/grats/deploys/6545788d5d5dde00088a523d
😎 Deploy Preview	https://deploy-preview-29--grats.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

captbaritone · 2023-11-03T15:38:15Z

TODO.md

@@ -50,7 +50,6 @@
 - [ ] Improve playground
  - Could we actually evaluate the resolvers? Maybe in a worker?
  - Could we hook up GraphiQL 2?
- [ ] Can we ensure the context and ast arguments of resolvers are correct?


Maybe premature to add this since we don't yet typecheck the info argument.

captbaritone · 2023-11-03T15:38:33Z

src/Extractor.ts

@@ -980,7 +985,7 @@ export class Extractor {
    } else if (node.kind === ts.SyntaxKind.FalseKeyword) {
      return this.gql.boolean(node, false);
    } else if (ts.isObjectLiteralExpression(node)) {
-      return this.cellectObjectLiteral(node);
+      return this.collectObjectLiteral(node);


Unrelated typo fix.

mandx

Very exciting this change! I was thinking (maybe you though this too) Grats could also support the @gqlContext docblock tag, so when present, all ctx arguments' type must resolve to that, otherwise, use this checking strategy as fallback.

mandx · 2023-11-03T15:48:52Z

src/tests/fixtures/resolver_context/MultipleContextValuesUsed.invalid.ts.expected

+-----------------
+OUTPUT
+-----------------
+src/tests/fixtures/resolver_context/MultipleContextValuesUsed.invalid.ts:16:34 - error: Context argument's type does not match. Grats expects all resolvers that read the context argument to use the same type for that argument. Did you use the incorrect type in one of your resolvers?


I think we should explain that Grats needs the exact same type reference (instead of structurally equivalent types) because it doesn't perform typechecking on the ctx arguments' type. Any user with TS experience will look at GratsContext and AlsoGratsContext and think "but, they are the same!" but "same" in this context means different things when Grats is checking the schema and when TS checks the code.

mandx · 2023-11-03T15:51:32Z

src/tests/fixtures/resolver_context/ContextValueTypedAsString.invalid.ts.expected

+-----------------
+src/tests/fixtures/resolver_context/ContextValueTypedAsString.invalid.ts:4:30 - error: Expected context parameter's type to be a type reference Grats validates that your context parameter is type-safe by checking all context values reference the same type declaration.
+
+4   greeting(args: never, ctx: string): string {


Is not supporting string intentional? Or is it because some technical limitation?

Technical limitation because we are tracking the definition of the type, and primitives and literals won't work.

mandx · 2023-11-03T15:52:37Z

src/tests/fixtures/resolver_context/ContextValueTypedAsNever.ts.expected

+-----------------
+src/tests/fixtures/resolver_context/ContextValueTypedAsNever.ts:4:30 - error: Expected context parameter's type to be a type reference Grats validates that your context parameter is type-safe by checking all context values reference the same type declaration.
+
+4   greeting(args: never, ctx: never): string {


Is it not possible to allow unknown as the type for ctx?

We could, just haven't added it yet! Unclear if it's needed though. I guess if you need to access the info argument after the context value and you don't actually ever use the context value?

Yeah, that's one use case. It would be a just escape hatch, in case we need to add the context argument (for whatever reason) but we don't want to deal with typing the context right now.

However, if it's easy to add, we could not add it now, and wait for (less theoretical, more practical) reasons to add it and make a better decision then.

captbaritone · 2023-11-03T17:22:47Z

Very exciting this change! I was thinking (maybe you though this too) Grats could also support the @gqlContext docblock tag, so when present, all ctx arguments' type must resolve to that, otherwise, use this checking strategy as fallback.

Yeah, I had a similar thought but more sequential than options. We can start here, and if we end up needing to actually know which value is the context value (say for positional arguments) we could layer on the @gqlContext tag as a new requirement. It would be a breaking change, but it would just mean adding a single tag to your single context type definition.

mandx · 2023-11-03T17:38:10Z

Very exciting this change! I was thinking (maybe you though this too) Grats could also support the @gqlContext docblock tag, so when present, all ctx arguments' type must resolve to that, otherwise, use this checking strategy as fallback.

Yeah, I had a similar thought but more sequential than options. We can start here, and if we end up needing to actually know which value is the context value (say for positional arguments) we could layer on the @gqlContext tag as a new requirement. It would be a breaking change, but it would just mean adding a single tag to your single context type definition.

I think all modes are valid, it just adds some complexity to the checking code:

If a positional arguments are found, then @gqlContext is required to be defined, and all present context args must reference that type.
If no positional args are found, but @gqlContext is defined, then all present context args must reference that type.
If no positional args are found, and @gqlContext is not defined, then just check that all present context args must reference that the same type, whatever that is.

I can see this working great, as long as if a check fails the error message is descriptive enough to explain what and why, and provide actions or suggestions.

captbaritone · 2023-11-03T22:48:35Z

src/Errors.ts

@@ -161,11 +161,11 @@ export function typeNameDoesNotMatchExpected(expected: string) {
 }

 export function argumentParamIsMissingType() {
-  return "Expected GraphQL field arguments to have a TypeScript type. If there are no arguments, you can use `args: never`.";
+  return "Expected GraphQL field arguments to have a TypeScript type. If there are no arguments, you can use `args: unknown`.";


This check/guidance was wrong before, but I'm fixing it here. Should have been unknown for this use case not never.

captbaritone · 2023-11-03T22:49:31Z

website/docusaurus.config.js

@@ -22,7 +22,7 @@ const config = {
  projectName: "grats", // Usually your repo name.

  onBrokenLinks: "throw",
-  onBrokenMarkdownLinks: "warn",
+  onBrokenMarkdownLinks: "throw",


Errors were turning up in CI but not local dev. This fixes that.

captbaritone added 3 commits November 2, 2023 16:07

[WIP] Validate the type of context argument

a4e62e8

Fix broken test

fd74e79

Check that all context types reference the same type

75255ac

captbaritone commented Nov 3, 2023

View reviewed changes

mandx reviewed Nov 3, 2023

View reviewed changes

captbaritone added 2 commits November 3, 2023 15:31

Allow unknown for ctx and prefer unknown for args over never

03dd662

Fix links (and another never -> unknown update)

8ee1d65

captbaritone commented Nov 3, 2023

View reviewed changes

captbaritone merged commit 6a72239 into main Nov 3, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate that all context types match #29

Validate that all context types match #29

captbaritone commented Nov 3, 2023

netlify bot commented Nov 3, 2023 •

edited

Loading

captbaritone Nov 3, 2023

captbaritone Nov 3, 2023

mandx left a comment

mandx Nov 3, 2023

mandx Nov 3, 2023

captbaritone Nov 3, 2023

mandx Nov 3, 2023

captbaritone Nov 3, 2023

mandx Nov 3, 2023

captbaritone commented Nov 3, 2023

mandx commented Nov 3, 2023

captbaritone Nov 3, 2023

captbaritone Nov 3, 2023

Validate that all context types match #29

Validate that all context types match #29

Conversation

captbaritone commented Nov 3, 2023

netlify bot commented Nov 3, 2023 • edited Loading

✅ Deploy Preview for grats ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captbaritone commented Nov 3, 2023

mandx commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Nov 3, 2023 •

edited

Loading