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

usage reporting: don't throw errors if willResolveField is called "late" #6398

Merged
merged 1 commit into from
Jun 4, 2022

Conversation

glasser
Copy link
Member

@glasser glasser commented May 6, 2022

The comment explains this in detail. Basically, this "shouldn't happen"
error actually could happen. In theory, the times it could happen are
the exact times that the error itself would be swallowed rather than
becoming visible... but a graphql-js bug meant that sometimes they would
become visible anyway.

Fixes #4472.

@glasser glasser requested a review from trevor-scheer May 6, 2022 06:35
@netlify
Copy link

netlify bot commented May 6, 2022

Deploy Preview for apollo-server-docs canceled.

Name Link
🔨 Latest commit 3fd72b1
🔍 Latest deploy log https://app.netlify.com/sites/apollo-server-docs/deploys/629aa9811009520008bb1f7b

@glasser
Copy link
Member Author

glasser commented May 6, 2022

I know I wrote a lot of details in this comment but I'm not sure that it's particularly clear.

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 6, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3fd72b1:

Sandbox Source
Apollo Server Typescript Configuration
Apollo Server Configuration
Apollo Server Issue #4472

Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

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

LGTM and the "confusing" comment actually explains the issue perfectly. And thanks for including the relevant graphql PR, I subscribed so we can follow along and hopefully go back to throwing an error in the future!

Include PR number as well in the changelog maybe?

The comment explains this in detail. Basically, this "shouldn't happen"
error actually could happen. In theory, the times it could happen are
the exact times that the error itself would be swallowed rather than
becoming visible... but a graphql-js bug meant that sometimes they would
become visible anyway.

Fixes #4472.
@glasser glasser force-pushed the glasser/willresolvefield-after-stoptiming branch from c555f27 to 3fd72b1 Compare June 4, 2022 00:38
@glasser glasser enabled auto-merge (squash) June 4, 2022 00:38
@glasser glasser merged commit 4c8f100 into main Jun 4, 2022
@glasser glasser deleted the glasser/willresolvefield-after-stoptiming branch June 4, 2022 00:42
@maxnachlinger
Copy link
Contributor

Would you fine humans be open to a PR back-porting this to v2 ?

@glasser
Copy link
Member Author

glasser commented Jul 8, 2022

No, Apollo Server v2 is no longer maintained other than for security issues. AS3 was released exactly a year ago today, and we're getting near to v4. It's time to upgrade!

@maxnachlinger
Copy link
Contributor

Oh I agree :) I work on a large federated graph and it's hard to compel disparate sub-graph teams to upgrade to a new major version. So it goes.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[apollo-engine-reporting] : willResolveField called after stopTiming!
3 participants