Defend against non-serializable params in `invariantWrappers`

henryqdineen commented 4 months ago

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:

[x] 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)
[x] Make sure all of the significant new logic is covered by tests

netlify[bot] commented 4 months ago

Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name	Link
Latest commit	233404e17b40116a23ad3ad9b1a9edc387a8d04f

changeset-bot[bot] commented 4 months ago

🦋 Changeset detected

Latest commit: 233404e17b40116a23ad3ad9b1a9edc387a8d04f

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 commented 4 months ago

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!

apollographql / apollo-client