Skip to content

cf modules e2e test #1814

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 30, 2022
Merged

cf modules e2e test #1814

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
81cdcdd
trying
saihaj Sep 28, 2022
efcf519
fixes
dotansimha Sep 28, 2022
f8f1dd3
trigger ci
dotansimha Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion e2e/tests/create-cf-deployment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export function createCFDeployment(
'utf-8',
),
module: isModule,
secretTextBindings: [
plainTextBindings: [
{
name: 'GRAPHQL_ROUTE',
text: `/${stackName}`,
Expand Down
37 changes: 21 additions & 16 deletions examples/cloudflare-modules/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,27 @@
// src/index.mjs
import { createYoga, createSchema } from 'graphql-yoga'

const { fetch } = createYoga({
graphqlEndpoint: '/graphql',
landingPage: false,
schema: createSchema({
typeDefs: /* GraphQL */ `
type Query {
greetings: String
}
`,
resolvers: {
Query: {
greetings: () =>
'This is the `greetings` field of the root `Query` type',
},
const schema = createSchema({
typeDefs: /* GraphQL */ `
type Query {
greetings: String
}
`,
resolvers: {
Query: {
greetings: () => 'This is the `greetings` field of the root `Query` type',
},
}),
},
})

export default { fetch }
export default {
fetch(request: Request, env: { [key: string]: string }) {
const yoga = createYoga({
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove the parameter and modify the bundled code inside the deployment script;
https://github.com/dotansimha/graphql-yoga/pull/1812/files
Or do you think it is too tricky?

Copy link
Member

Choose a reason for hiding this comment

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

I feel like it's too tricky... see my other comments on why we need this setup

Copy link
Member

Choose a reason for hiding this comment

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

But this is also our example for CF Modules and useLandingPage and having yoga in the fetch function etc are too much for a basic example. We can have another project for e2e tests but we will lose the ability of testing the actual example in this case.

Copy link
Member

Choose a reason for hiding this comment

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

I see, so let's move on with this PR as-is. Then, we'll open an issue to improve the setup. We'll need to experiment with using the router config that includes *, then we can use /WORKER_ID/graphql and keep the landing page enabled.

graphqlEndpoint: env.GRAPHQL_ROUTE || '/graphql',
Copy link
Member

Choose a reason for hiding this comment

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

We do landingPage: false and then GRAPHQL_ROUTE is injected to tell the Yoga server to expect incoming connections on e2e.graphql-yoga.com/WORKER_ID
This way we can have a public endpoint easily, without the need to capture /WORKER_ID/*.
It's a workaround, but it works for now...

Copy link
Member

Choose a reason for hiding this comment

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

Also, CF module Worker doesn't use the globals for env vars, only through the env provided as part of fetch, so we need a thin wrapper in this case

Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding those extra options to our example, we can make it work by modifying the bundle like here;
#1812
What do you think @dotansimha @saihaj ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we can do that too

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the tricky part of it is just that is mutates the code example

landingPage: false,
schema,
})

return yoga.fetch(request)
},
}