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

Add @schema decorator to mark namespaces as GraphQL schemas #1

Open
wants to merge 1 commit into
base: feature/graphql
Choose a base branch
from

Conversation

steverice
Copy link

Using the TypeSpec.GraphQL.@schema decorator on a namespace indicates that the decorated namespace represents a GraphQL schema that should be generated by the GraphQL emitter.

Because this allows for multiple schemas to be specified in a TypeSpec source, our test host is reworked to provide a GraphQLSchemaRecord corresponding to each schema produced.

This commit does not actually implement any emitter functionality, but populates a state map that will be used by the emitter in the future.

@steverice
Copy link
Author

Note to reviewers: the first commit is just some package.json metadata that I have proposed directly upstream: microsoft#5076

Copy link

@AngelEVargas AngelEVargas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose my personal GH user is already working!

Comment on lines 14 to 15
// This will set the namespace for all the decorators
export const namespace = "TypeSpec.GraphQL";
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// This will set the namespace for all the decorators
export const namespace = "TypeSpec.GraphQL";

This should not be needed when using export const $decorators.

Copy link
Author

@steverice steverice Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out we do need this. When we import schema.js into schema.tsp, the definition must be namespaced in order to match the TypeSpec declaration.

So we have two different paths for namespacing:

  1. export const $decorators in tsp-index.js sets the namespace explicitly for all decorators when used by external code
  2. export const namespace = in schema.js sets the namespace when the $schema decorator is imported directly.

Since these are two independent import paths, they should not conflict.

@steverice steverice force-pushed the schema-decorator branch 4 times, most recently from bb18a3a to c0fff8a Compare November 18, 2024 22:35
packages/graphql/package.json Outdated Show resolved Hide resolved
packages/graphql/package.json Outdated Show resolved Hide resolved
packages/graphql/package.json Show resolved Hide resolved
@@ -1,10 +1,26 @@
{
"name": "@typespec/graphql",
"version": "0.1.0",
"author": "Microsoft Corporation",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned, these package.json updates have been proposed upstream on their own. So I am hoping to get MSFT's input on what should and shouldn't be in package.json as I think it's ultimately up to them.

Until that, I am mostly just copying things over from other packages (primarily openapi3).

packages/graphql/src/lib.ts Outdated Show resolved Hide resolved
packages/graphql/lib/schema.tsp Show resolved Hide resolved
packages/graphql/src/lib/schema.ts Outdated Show resolved Hide resolved
packages/graphql/src/testing/utils.ts Outdated Show resolved Hide resolved
packages/graphql/test/schema.test.ts Outdated Show resolved Hide resolved
// Change this to whatever makes sense for the actual GraphQL emitter, probably a GraphQLSchemaRecord
return [content, diagnostics];
const schema = content
? buildSchema(content, {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can run the emitter in test mode to just return the GraphQL AST directly without deserializing the content into the AST using buildSchema

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I wanted to do this, but the structure of the test host and runner built into @typespec/compiler doesn't really allow for this. So after looking into it for a while I gave up.

This approach matches what is done with the OpenAPI tests — the file is emitted, and then loaded and JSON parsed into the OpenAPI object for tests to use.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! Can we add this as a comment here and maybe Brian Terlson can chime in?

Copy link

@swatkatz swatkatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small comment to add the test mode thing as a comment to the test runner.

// Change this to whatever makes sense for the actual GraphQL emitter, probably a GraphQLSchemaRecord
return [content, diagnostics];
const schema = content
? buildSchema(content, {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! Can we add this as a comment here and maybe Brian Terlson can chime in?

@steverice steverice force-pushed the schema-decorator branch 2 times, most recently from 9b12057 to f87595e Compare November 20, 2024 22:52
Using the `TypeSpec.GraphQL.@schema` decorator on a namespace indicates that the decorated namespace represents a GraphQL schema that should be generated by the GraphQL emitter.

Because this allows for multiple schemas to be specified in a TypeSpec source, our test host is reworked to provide a `GraphQLTestResult` corresponding to each schema produced.

This commit does not actually implement any emitter functionality, but populates a state map that will be used by the emitter in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

3 participants