Skip to content

Conversation

@cf-ypark
Copy link
Contributor

This is for GraphQL MCP Server.

image

@cf-ypark
Copy link
Contributor Author

cf-ypark commented May 13, 2025

@Maximo-Guk can i get a review and get this merged? Do i need to get it deployed first? I'm still in the middle of getting the oauth for staging/prod.

@Maximo-Guk
Copy link
Member

@Maximo-Guk can i get a review and get this merged? Do i need to get it deployed first? I'm still in the middle of getting the oauth for staging/prod.

Changes look good to me! Just left a few minor comments, yes it would be great if you could manually deploy it first! Let me know if I can be of any help with deploying!

@cf-ypark
Copy link
Contributor Author

@Maximo-Guk can i get a review and get this merged? Do i need to get it deployed first? I'm still in the middle of getting the oauth for staging/prod.

Changes look good to me! Just left a few minor comments, yes it would be great if you could manually deploy it first! Let me know if I can be of any help with deploying!

Cool. thanks I'll go through your comment. I did deploy already to staging/production and seems working good.

@cf-ypark
Copy link
Contributor Author

@Maximo-Guk i went through all the comments (except one thing). wdyt?

@Maximo-Guk
Copy link
Member

Maximo-Guk commented May 13, 2025

@Maximo-Guk i went through all the comments (except one thing). wdyt?

lgtm! ( assuming CI passes )

@Maximo-Guk
Copy link
Member

Maximo-Guk commented May 13, 2025

Looks like we just need to fix CI ( unfortunately this PR comes from a fork so I had to push it up to a branch on mcp-server-cloudflare repo to get CI to run, but you can test linting locally by running the same commands that CI runs )

Also, we should update the top level README.md to include this new server!

MCP_SERVER_VERSION: "<PLACEHOLDER>" | "1.0.0";
CLOUDFLARE_CLIENT_ID: string;
CLOUDFLARE_CLIENT_SECRET: string;
MCP_OBJECT: DurableObjectNamespace<import("./src/graphql.app").GraphQLMCP>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Maximo-Guk any clue why I get this line 3-16, when i ran npx wrangler types? I don't see this generated for other apps.

Copy link
Member

@Maximo-Guk Maximo-Guk May 14, 2025

Choose a reason for hiding this comment

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

We should use pnpm types https://github.com/cloudflare/mcp-server-cloudflare/pull/158/files#diff-3081a69eb973512fc1107c07bfad799453d553915ddd2b0b512c5371ee1f4f52R11 which runs wrangler types --include-env=false omitting the env's from the codegenerated types file ( since we already have the context file that contains the types for the env's )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh gotcha

@cf-ypark cf-ypark force-pushed the graphql-mcp branch 2 times, most recently from a094bc9 to e3cad4e Compare May 14, 2025 15:56
@Maximo-Guk Maximo-Guk merged commit ecbeee8 into cloudflare:main May 14, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants