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

Defend against non-serializable params in invariantWrappers #11861

Merged
merged 4 commits into from
Jun 26, 2024

Conversation

henryqdineen
Copy link
Contributor

@henryqdineen henryqdineen commented May 20, 2024

Apologies for the lack of reproduction. I help maintain a large Apollo codebase spread across hundreds of repos that is in the process of upgrading to latest. In a couple instances recently we have run into errors where stringifyForDisplay() has blown up for not being able to serialize one of the params.

Example 1: We have a custom link that takes the ApolloClient instance. Something like below (I know it's really weird):

class FooClient extends ApolloClient {
  constructor() {
    // ...
    this.setLink(new FooLink({ client: this }));  
  }
}

We had to tweak this code to do some link composition and the code blew up on the You are calling concat on a terminating link, which will have no effect... invariant because the FooLink instance had circular references. I know this was user ultimately user error but the error message was not useful.

Example 2: We have a custom terminating HttpLink that throws our own errors instead of the normal fetch() error. For ugly legacy reasons these error cannot be JSON stringified and can cause a SecurityError. When using client.refetchQueries() if an error occurs we will run into the 'In client.refetchQueries, Promise.all promise rejected with error...' invariant and it blows up because networkError is not serializable.

I totally understand how / why neither of these issues should happen in typical Apollo Client usage but I figured I would make this PR to see if it might be useful upstream. We will likely patch-package Apollo Client as a workaround. I know there are "safe" JSON stringify implementations out there but since this is such an edge case I with with a simple try/catch fallback.

Thanks and let me know if you have any questions.

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

Copy link

netlify bot commented May 20, 2024

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 233404e

Copy link

changeset-bot bot commented May 20, 2024

🦋 Changeset detected

Latest commit: 233404e

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

This PR includes changesets to release 1 package
Name Type
@apollo/client 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

@alessbell
Copy link
Member

Hey @henryqdineen 👋

Thanks for opening this PR - I can see how this would be useful in the cases you've outlined. I'll take it to the team to discuss as we're not all online today and we'll get back to you as soon as we can. Thanks!

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

Hey @henryqdineen 👋

So sorry for the delay on this review! This PR got away from us.

This change looks great. We'll get this out with the next patch release. Thanks so much for the contribution!

@jerelmiller jerelmiller merged commit 1aed0e8 into apollographql:main Jun 26, 2024
38 checks passed
@github-actions github-actions bot mentioned this pull request Jun 26, 2024
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.

3 participants