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

feat(package/gqty): Use GraphQLError over plain objects in defaultResponseHandler #2058

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vicary
Copy link
Member

@vicary vicary commented Jan 26, 2025

No description provided.

@vicary vicary force-pushed the feat/graphql-errors branch from d2c86c6 to 97371e8 Compare January 26, 2025 07:07
@vicary vicary self-assigned this Jan 26, 2025
Copy link
Contributor

github-actions bot commented Jan 26, 2025

🚀 Snapshot Release (canary)

The latest changes of this PR are available as canary on npm (based on the declared changesets):

Package Version Info
@gqty/cli 4.2.2-canary-20250131112755.43462e94e3bde969ca87ef505b89c46a22cec873 npm ↗︎ unpkg ↗︎
gqty 3.3.0-canary-20250131112755.43462e94e3bde969ca87ef505b89c46a22cec873 npm ↗︎ unpkg ↗︎
@gqty/logger 4.0.0-canary-20250131112755.43462e94e3bde969ca87ef505b89c46a22cec873 npm ↗︎ unpkg ↗︎
@gqty/react 4.0.0-canary-20250131112755.43462e94e3bde969ca87ef505b89c46a22cec873 npm ↗︎ unpkg ↗︎
@gqty/solid 1.0.0-canary-20250131112755.43462e94e3bde969ca87ef505b89c46a22cec873 npm ↗︎ unpkg ↗︎

@vicary vicary force-pushed the feat/graphql-errors branch from 97371e8 to c5d0c33 Compare January 26, 2025 07:10
@vicary vicary force-pushed the feat/graphql-errors branch from c5d0c33 to b2c95fd Compare January 26, 2025 07:14
Copy link

@kleberbaum kleberbaum left a comment

Choose a reason for hiding this comment

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

Does seem to work:
image
Throws error like expected:
image

As the object is now a array of new GraphQLError shouldn't be re-throwing it like this also become possible?:
image

In my tests I wasn't able to get any positive Results for it tho:
image

@vicary
Copy link
Member Author

vicary commented Jan 27, 2025

@kleberbaum Is it possible that Pylon is using the error trace for location/path look up?

In that case a new Error must be created at the callsite of a resolver, if that works it also begs the question of why new Error at query fetcher also works with Pylon.

@kleberbaum
Copy link

@kleberbaum Is it possible that Pylon is using the error trace for location/path look up?

In that case a new Error must be created at the callsite of a resolver, if that works it also begs the question of why new Error at query fetcher also works with Pylon.

Good question tbh idk. Lets ask @schettn.

@kleberbaum
Copy link

I kinda think that all fetch errors including "Network Errors" should also be returned as GQtyError, but but with empty GraphQLError Array. Currently they are just any error.
image
image

@vicary
Copy link
Member Author

vicary commented Jan 27, 2025

It's simply because the design choice of a generated client, where network errors are throwing at fetch() before response handler have a chance to process it.

We want the query fetcher to stay highly customizable rather than a blackbox client. You may have to catch network errors at fetch() for the moment.

Your idea definitely helps with the design of the upcoming modular/plugin system though.

@schettn
Copy link

schettn commented Jan 27, 2025

Does seem to work: image Throws error like expected: image

As the object is now a array of new GraphQLError shouldn't be re-throwing it like this also become possible?: image

In my tests I wasn't able to get any positive Results for it tho: image

@vicary @kleberbaum What is the instance of the error.graphqlErrors[0]. It has to be GraphQLError, if not the error won't be returned because of error masking. https://the-guild.dev/graphql/yoga-server/docs/features/error-masking.

So are graphqlErrorsjust plain objects or real errors?

@schettn
Copy link

schettn commented Jan 27, 2025

@kleberbaum You may check it with console.log(error.graphqlErrors[0] instanceof GraphQLError)

@kleberbaum
Copy link

Yes.
406773396-f971cc2a-3819-415e-812e-5a3e3dd5e787


if (Array.isArray(result?.errors)) {
result.errors = result.errors.map(
(error: any) => new GraphQLError(error.message, error)
Copy link

@kleberbaum kleberbaum Jan 27, 2025

Choose a reason for hiding this comment

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

GraphQLError Array

Copy link

@kleberbaum kleberbaum left a comment

Choose a reason for hiding this comment

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

At the moment I am only on the phone but I already have a test environment. Will push it on getcronit fork.

@schettn
Copy link

schettn commented Jan 27, 2025

@kleberbaum Double check it with the console log.

If it's true and still not working there could be a version mismatch. (Maybe a different graphql version is installed for gqty and pylon.)

@kleberbaum
Copy link

kleberbaum commented Jan 27, 2025

@kleberbaum Double check it with the console log.

If it's true and still not working there could be a version mismatch. (Maybe a different graphql version is installed for gqty and pylon.)

True. The issue was that there was a mismatch between GraphQL versions. Therefore, an instance of GraphQLError is not an instance of GraphQLError and can't be thrown:
"graphql": "^16.10.0" and "graphql": "^16.9.0"

It was solved by forcing "16.9.0" for the pnpm workspace:
image_2025-01-27_22-35-46

image
Unfortunately, it is always possible that the GraphQL version of a project is not the same as in GQty and then the error will be masked by Apollo.

@vicary
Copy link
Member Author

vicary commented Jan 28, 2025

graphql is only a peer dependency of gqty, package managers should be able to dedupe them.

@vicary
Copy link
Member Author

vicary commented Jan 28, 2025

@kleberbaum Could resolve the version conflict without using explicit overrides? We can merge this one so I can move on to the pylon schema.

@kleberbaum
Copy link

@kleberbaum Could resolve the version conflict without using explicit overrides? We can merge this one so I can move on to the pylon schema.

Wasn't able to verify yet if it works in another environment. I was only able to confirm that In the gqty repo pnpm workspace it is not working without the override if the project using GQty is adding a different graphql version in package json.

@vicary vicary force-pushed the feat/graphql-errors branch 7 times, most recently from 4e590e6 to 8afcf2c Compare January 31, 2025 10:04
@vicary vicary force-pushed the feat/graphql-errors branch from 8afcf2c to 43462e9 Compare January 31, 2025 11:27
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.

3 participants