-
Notifications
You must be signed in to change notification settings - Fork 0
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
Set up GraphQL emitter skeleton #2
base: feature/graphql
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
import { navigateProgram, type Program, type SemanticNodeListener } from "@typespec/compiler"; | ||
import type { GraphQLObjectType } from "graphql"; | ||
|
||
type Mutable<T> = { | ||
-readonly [k in keyof T]: T[k]; | ||
}; | ||
|
||
// This class contains the registry of all the GraphQL types that are being used in the program | ||
export class GraphQLTypeRegistry { | ||
program: Program; | ||
readonly programNavigated: boolean = false; | ||
|
||
constructor(program: Program) { | ||
this.program = program; | ||
return new Proxy(this, { | ||
get(target: GraphQLTypeRegistry, prop: string, receiver) { | ||
if (GraphQLTypeRegistry.#publicGetters.includes(prop)) { | ||
if (!target.programNavigated) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would there be a situation where the program has been navigated? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the longer discussion below. Basically, the intent here is that |
||
const mutableThis = target as Mutable<GraphQLTypeRegistry>; | ||
navigateProgram(target.program, target.#semanticNodeListener); | ||
mutableThis.programNavigated = true; | ||
} | ||
} | ||
return Reflect.get(target, prop, receiver); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't seen the use of reflection in the TSP repo. I am not sure that this is good typescript practice. I am not an expert on this, but in compiled languages at least using reflection is usually a code smell. Besides, I am not quite convinced that this is needed as per my comment below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The way we're using it here is how it's expected to be used — when you are creating a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, this actually is used in the TypeSpec repo. It's in the "experimental" |
||
}, | ||
}); | ||
} | ||
|
||
static get #publicGetters() { | ||
return Object.entries(Object.getOwnPropertyDescriptors(GraphQLTypeRegistry.prototype)) | ||
.filter(([key, descriptor]) => { | ||
return typeof descriptor.get === "function" && key !== "constructor"; | ||
}) | ||
.map(([key]) => key); | ||
} | ||
|
||
get rootQueryType(): GraphQLObjectType | undefined { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to make this dynamic/lazy? We can use I think a more readable and deterministic navigate flow looks like navigateProgram --> enterXXX extract information based on type and add to some map --> exitXXX extract information from map and create a GraphQL AST object. None of the existing emitters need lazy initialization, so I am not sure that the lazy init is doing much for us here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The getters in the prototype are only used in the exit method, so these shouldn't really be public. Maybe registry is not the right name, but this is the thing that should return a schema from a public method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great! I was hoping to have this discussion 🙂 The only thing that's intended to be lazy in The emitter flow then looks something like this:
This differs from the flow you outlined in that there is no explicit "now we must navigate the program" step. Instead of it being a TypeSpec-centric approach, it is a GraphQL-centric approach. This matters when we are looking to emit multiple GraphQL schemas — we want to start with each schema that we are going to emit and figuring out what it looks like, rather than starting with the whole TypeSpec program and figuring out what schemas we need to create, then categorizing all of the data into those different schemas (which requires a lot of global state). We already know what schemas we need to emit from the From my understanding of the TypeSpec code, this is more in line with how both the existing Also — I fully intend/expect that we will use |
||
return; | ||
} | ||
|
||
// This is the listener based on navigateProgram that will walk the TSP AST and register the types, | ||
// deferred in some cases, and then materialize them in exitXXX functions | ||
get #semanticNodeListener(): SemanticNodeListener { | ||
return {}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import { isType, type GraphQLType } from "graphql"; | ||
import { expect } from "vitest"; | ||
|
||
interface GraphQLAssertions<R = unknown> { | ||
toEqualType: (expected: GraphQLType) => R; | ||
} | ||
|
||
declare module "vitest" { | ||
interface Assertion<T = any> extends GraphQLAssertions<T> {} | ||
interface AsymmetricMatchersContaining extends GraphQLAssertions {} | ||
} | ||
|
||
expect.extend({ | ||
toEqualType(received: GraphQLType, expected: GraphQLType) { | ||
if (!isType(expected)) { | ||
return { | ||
pass: false, | ||
message: () => `Expected value ${expected} is not a GraphQLType.`, | ||
}; | ||
} | ||
|
||
if (!isType(received)) { | ||
return { | ||
pass: false, | ||
message: () => `Received value ${received} is not a GraphQLType.`, | ||
}; | ||
} | ||
|
||
const { isNot } = this; | ||
return { | ||
pass: received.toJSON() === expected.toJSON(), | ||
message: () => `${received} is${isNot ? " not" : ""} the same as ${expected}`, | ||
}; | ||
}, | ||
}); | ||
|
||
export { expect }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { expectDiagnosticEmpty } from "@typespec/compiler/testing"; | ||
import { GraphQLSchema, printSchema } from "graphql"; | ||
import { describe, it } from "vitest"; | ||
import { GraphQLEmitter } from "../src/schema-emitter.js"; | ||
import { expect } from "./assertions.js"; | ||
import { emitSingleSchemaWithDiagnostics } from "./test-host.js"; | ||
|
||
describe("GraphQL emitter", () => { | ||
it("Can produce a placeholder GraphQL schema", async () => { | ||
const result = await emitSingleSchemaWithDiagnostics(""); | ||
expectDiagnosticEmpty(result.diagnostics); | ||
expect(result.graphQLSchema).toBeInstanceOf(GraphQLSchema); | ||
expect(result.graphQLSchema?.getQueryType()).toEqualType(GraphQLEmitter.placeholderQuery); | ||
}); | ||
|
||
it("Can produce an SDL output", async () => { | ||
const result = await emitSingleSchemaWithDiagnostics(""); | ||
expectDiagnosticEmpty(result.diagnostics); | ||
expect(result.graphQLOutput).toEqual(printSchema(result.graphQLSchema!)); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import { beforeEach, describe, it } from "vitest"; | ||
import { GraphQLTypeRegistry } from "../src/registry.js"; | ||
import { expect } from "./assertions.js"; | ||
import { createGraphqlTestRunner } from "./test-host.js"; | ||
|
||
describe("GraphQL Type Registry", () => { | ||
let registry: GraphQLTypeRegistry; | ||
|
||
beforeEach(async () => { | ||
const runner = await createGraphqlTestRunner(); | ||
await runner.diagnose(""); | ||
registry = new GraphQLTypeRegistry(runner.program); | ||
}); | ||
|
||
it("Will navigate program when state is accessed", () => { | ||
expect(registry.programNavigated).toBe(false); | ||
expect(registry.rootQueryType).toBeUndefined(); | ||
expect(registry.programNavigated).toBe(true); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steverice there is a version of this in
@typespec/compiler/src/utils/misc.ts
that we can probably use instead.