do not return 404 when using query params or trailing slash

[nescalante]

this PR fixes two errors:

• when using graphiql and then writing a query, graphiql adds "?query=your+query" to the query params. When refreshing the page this is returning 404 since the "unhandled route" validation is not taking query params into account.
• in the same line, the same validation is throwing 404 when using trailing slashes in the url

I added a test for these two cases and then fixed them

[changeset-bot]

🦋 Changeset detected

Latest commit: 7aba56b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages

Name	Type
graphql-yoga	Patch
apollo-federation-gateway-with-yoga	Patch
apollo-subgraph-with-yoga	Patch
graphql-lambda	Patch
cloudflare-advanced	Patch
cloudflare	Patch
functions	Patch
hackernews	Patch
nextjs-app	Patch
hello-world-benchmark	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ardatan]

Let's hold this one. That "if" condition is important for the performance. Maybe the path needs to be normalized in whatwg-node instead

[n1ru4l]

@nescalante What is the use-case of using a trailing slash for the graphql endpoint? Why would you want to use both /graphql and /graphql/?

In case you want to only use /graphql/, you could instead use the graphqlEndpoint option passed to createYoga, for specifying /graphql/ as your graphql endpoint.

[nescalante]

@n1ru4l basically in general sites "ignore" the trailing slash so both https://github.com/dotansimha/graphql-yoga/ and github.com/dotansimha/graphql-yoga are valid urls. Express also supports both URL with trailing and without trailing slash.

[nescalante]

@ardatan regarding performance, would you prefer this alternative without regexes?

[n1ru4l]

@nescalante Wouldn't that indicate duplicated routes, shouldn't a trailing slash instead return a 301 with the location without the trailing slash?

i changed my mind

[nescalante]

ok, two questions then:

• what do you think about passing a custom validation for properties? so instead of having a fixed string for the graphqlEndpoint, a function that validates that the graphqlEndpoint is ok
• would you be ok with separating the query parameters fix as a separate thing?

if you agree with approach 1, then I will be happy to discuss the right way to provide this option.

My main concern is that validations over URL are too strict, and I can't handle them. So I can't have graphql over two endpoints for example, or graphql in one endpoint, and graphiql in another one

[nescalante]

😴

[ardatan]

Could you add tests for trailing slash as well?

[nescalante]

closing this PR, not interested anymore

[Urigo]

Thank you @nescalante for the contribution and I'm very sorry we haven't got to review it in a timely manner...

We've now shipped this change thanks to another contribution from @antonio-iodice

We'll do better with swiftly reviewing changes from valuable contributors such as yourself
❤️