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

[v3] express.use() on custom path does not work without setting graphqlEndpoint #1901

Closed
Tracked by #1358
charlypoly opened this issue Oct 17, 2022 · 8 comments · Fixed by #2730
Closed
Tracked by #1358

[v3] express.use() on custom path does not work without setting graphqlEndpoint #1901

charlypoly opened this issue Oct 17, 2022 · 8 comments · Fixed by #2730

Comments

@charlypoly
Copy link
Contributor

The following code example returns 404 when querying /api/v1/graphql:

const app = express();

const yoga = createYoga({
	schema: // ...
});

app.use(`/api/v1/graphql`, yoga);

However, it works with the following:

const app = express();

const yoga = createYoga({
    graphqlEndpoint: `/api/v1/graphql`,
	schema: // ...
});

app.use(`/api/v1/graphql`, yoga);

If this is the expected behavior, we should document it 📖

@charlypoly charlypoly added the stage/2-failing-test A failing test was created that describes the issue label Oct 17, 2022
@theguild-bot theguild-bot mentioned this issue Oct 19, 2022
@ardatan ardatan removed the stage/2-failing-test A failing test was created that describes the issue label Nov 8, 2022
@ardatan ardatan changed the title [v3/express] express.use() on custom path does not work without setting graphqlEndpoint [v3] express.use() on custom path does not work without setting graphqlEndpoint Apr 20, 2023
@EmrysMyrddin
Copy link
Collaborator

I have added a failing test case.
The problem is not exactly what is shown in the given example. The bug appears only when the endpoint doesn't end with /graphql.

I'm working on a fix, but for now I'm not sure how I can do this without introducing a breaking change.

@ardatan
Copy link
Collaborator

ardatan commented Apr 28, 2023

You have to define graphqlEndpoint in createYoga.

app.use('/my-path', 
  createYoga({
     ....,
     graphqlEndpoint: '/my-path'
  })
)

@EmrysMyrddin
Copy link
Collaborator

EmrysMyrddin commented Apr 28, 2023

Yes, that's a workaround, but I'm trying to find a fix which doesn't require the developer to duplicate the endpoint setting.

If I don't find a good way to do it, I will just try to improve documentation to make it clear graphqlEndpoint is mandatory even when using external http server router.

@ardatan
Copy link
Collaborator

ardatan commented Apr 28, 2023

No it is not a workaround. It hasn't been documented yet, this is why we keep this issue open.

@EmrysMyrddin
Copy link
Collaborator

Ok, I will update the documentation then :-)
Thank you for the clarification !

@EmrysMyrddin
Copy link
Collaborator

@ardatan Do you think I have to add this information in every pages in the "Integrations" category ?

This was referenced May 7, 2024
This was referenced May 23, 2024
@Vince-Smith
Copy link

No it is not a workaround. It hasn't been documented yet, this is why we keep this issue open.

@ardatan I know this is closed, but I came across this recently and did a little digging and just wanted to confirm.

url.pathname !== args.graphqlEndpoint &&
url.pathname !== `${args.graphqlEndpoint}/` &&

Is there any reason that these calls couldn't be changed to:

 !url.pathname.endsWith(args.graphqlEndpoint) && 
 !url.pathname.endsWith(`${args.graphqlEndpoint}/`) && 

Reason being right now I don't really have a custom graphql endpoint, it's still the default of /graphql it's just nested in middlewares like /api/v1/graphql.

I would expect graphqlEndpoint to be used if I wanted to expose graphql somewhere like /foo or /api/v1/foo.

@ardatan
Copy link
Collaborator

ardatan commented Dec 17, 2024

I would expect graphqlEndpoint to be used if I wanted to expose graphql somewhere like /foo or /api/v1/foo.

But this is an assumption, when you provide /foo as graphqlEndpoint, then it will handle /foo, /somerandom/foo, and any combination ending with /foo which is an incorrect behavior. That's why it needs to be explicit.

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 a pull request may close this issue.

4 participants