Set up GraphQL emitter skeleton #2
Conversation
steverice
force-pushed
the
schema-structure
branch
from
880d7ac to
26261e3
Compare
steverice
force-pushed
the
schema-decorator
branch
from
c0fff8a to
4581ffb
Compare
steverice
force-pushed
the
schema-structure
branch
from
26261e3 to
b7f962d
Compare
steverice
force-pushed
the
schema-decorator
branch
3 times, most recently
from
f87595e to
5ef2ace
Compare
steverice
force-pushed
the
schema-structure
branch
from
b7f962d to
1666a80
Compare
| .map(([key]) => key); | ||
| } | ||
|
|
||
| get rootQueryType(): GraphQLObjectType | undefined { |
There was a problem hiding this comment.
Why do we need to make this dynamic/lazy? We can use stateMaps to handle state if we want (although we should check that with Microsoft and I'm not sure if that'll work for all our cases, but that is a different discussion). The navigateProgram already takes care of navigating the program, so I don't think we need a dynamically accessible get function.
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.
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.
Great! I was hoping to have this discussion 🙂
The only thing that's intended to be lazy in GraphQLTypeRegistry is the execution of navigateProgram. Basically, we have this object/interface that we want to be able to provide various information about the schema, but in order to have that information we need to have walked through the TSP program via navigateProgram. The intent of this Proxy trap is to ensure that, whenever some of that data is accessed, we have already navigated the program and therefore that data is available.
The emitter flow then looks something like this:
GraphQLEmitterknows how to construct a GraphQL schema — i.e. it knows that you start with 1-3 root object types, each of those object types have fields, each of those fields are of various types, etc.- As
GraphQLEmitterworks on constructing the schema, it asksGraphQLTypeRegistryvarious GraphQL-y questions like: "is there a root Query type?" "What fields are on this object type?" "What does this decorator look like?" GraphQLTypeRegistryinterprets the TypeSpec schema in order to provide answers to those questions. It does so by walking through the TSP program and storing the answers to the questions that it is capable of asking.
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 @schema decorator's stateMap.
From my understanding of the TypeSpec code, this is more in line with how both the existing TypeEmitter structure and the experimental TypeKit approaches work. They focus on creating definitions in the emitted language, and provide a series of questions that can be asked about the TypeSpec program in order to do so.
Also — I fully intend/expect that we will use stateMap for handling state where it can do so.
| 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.
Why would there be a situation where the program has been navigated? navigateProgram is deterministic in it's walk, so there is no reason to check for this.
There was a problem hiding this comment.
See the longer discussion below. Basically, the intent here is that navigateProgram is never called explicitly — it's an implementation detail.
| mutableThis.programNavigated = true; | ||
| } | ||
| } | ||
| return Reflect.get(target, prop, receiver); |
There was a problem hiding this comment.
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.
Reflect in JavaScript isn't really the same concept as reflection in other languages:
The major use case of Reflect is to provide default forwarding behavior in Proxy handler traps.
— JS docs
The way we're using it here is how it's expected to be used — when you are creating a Proxy object, it's used to call through to the original proxied object.
There was a problem hiding this comment.
Also, this actually is used in the TypeSpec repo. It's in the "experimental" TypeKit stuff, which is the new code that Timothee seems to have been working on as the replacement for the existing emitter framework. Since it's some of the newest code, I imagine it's up to the standards that the Microsoft team intends to use.
|
I would check with Microsoft whether using StateMap and projections is the right approach. I am not sure where they are with that. They hadn't raised using that when we first walked them through the code. |
| type Mutable<T> = { | ||
| -readonly [k in keyof T]: T[k]; | ||
| }; |
There was a problem hiding this comment.
@steverice there is a version of this in @typespec/compiler/src/utils/misc.ts that we can probably use instead.
This commit sets up the basic skeleton for the GraphQL emitter, capable of handling multiple defined GraphQL schemas. The approach here is slightly different from before. Here, we are limiting the functionality of `GraphQLEmitter` to handling the schema definitions, and not participating in any direct parsing of the TSP program. Instead, the `GraphQLTypeRegistry` is responsible for handling its own interpretation of the TSP program in order to provide the types in its registry. This primarily allows for two things: 1. The `GraphQLTypeRegistry` can encapsulate its own functionality, instead of being a "bucket of state" that must be modified and managed externally. 2. The "bucket of state" responsibility can be primarily handled by the StateMap library, with the `GraphQLTypeRegistry` being the orchestrator of that state The `GraphQLTypeRegistry` uses a `Proxy` object to ensure that the program navigation has taken place before any of its public properties are accessed.
steverice
force-pushed
the
schema-structure
branch
from
1666a80 to
6bd70d0
Compare
This commit sets up the basic skeleton for the GraphQL emitter, capable of handling multiple defined GraphQL schemas.
The approach here is slightly different from before. Here, we are limiting the functionality of
GraphQLEmitterto handling the schema definitions, and not participating in any direct parsing of the TSP program. Instead, theGraphQLTypeRegistryis responsible for handling its own interpretation of the TSP program in order to provide the types in its registry.This primarily allows for two things:
GraphQLTypeRegistrycan encapsulate its own functionality, instead of being a "bucket of state" that must be modified and managed externally.GraphQLTypeRegistrybeing the orchestrator of that stateThe
GraphQLTypeRegistryuses aProxyobject to ensure that the program navigation has taken place before any of its public properties are accessed.