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

MockLink doesn't strip undefined values | error message doesn't show undefined variables #6771

Closed
willrax opened this issue Aug 4, 2020 · 9 comments · Fixed by #8340
Closed
Assignees

Comments

@willrax
Copy link

willrax commented Aug 4, 2020

Intended outcome:
When making a request against a live GraphQL endpoint any undefined values in the variables object are removed from the payload.

When testing using the MockedProvider, the variables that were undefined remain in the request. This leads to the equality check failing (which it should).

The problem comes when you see the error message that Apollo throws.

The error message No more mocked responses were found etc uses JSON.stringify which excludes any values that were undefined. This behavior makes it tricky to debug because you're essentially seeing the exact mock you provided to the test.

Actual outcome:

  • The query shown in the network request is the same as the query received by the MockedProvider.
  • The error messages shows the query with all the values it's using to look for a mocked request.

How to reproduce the issue:

Codesandbox

To reproduce:

  • Run the tests and check the browser page.

In App.js note that the mutation has a value with undefined in the variables payload. Note that the mock in the test has the payload without the undefined value. This is the same request you would see get sent in the network tab.

Note that the error message excludes the undefined variable (see image) and that the query it's showing matches our mock exactly.

AOahOdkn@2x

Versions

@apollo/[email protected]

Chrome: 84.0.4147.105
Jest: 25.2.3

@willrax willrax changed the title MockLink doesn't strip undefined values. MockLink doesn't strip undefined values | error message doesn't show undefined variabesl Aug 4, 2020
@willrax willrax changed the title MockLink doesn't strip undefined values | error message doesn't show undefined variabesl MockLink doesn't strip undefined values | error message doesn't show undefined variables Aug 4, 2020
@willrax
Copy link
Author

willrax commented Aug 4, 2020

One solution i can think of is using a replacer function on JSON.stringify to attach another message the those values that are present in the request but are not present in the mock.

eg:

function replacer(key, value) {
    return typeof value === 'undefined' ? "SOME GOOD MESSAGE OR SOMETHING" : value;
}
    if (!response || typeof responseIndex === 'undefined') {
      this.onError(new Error(
        `No more mocked responses for the query: ${print(
          operation.query
        )}, variables: ${JSON.stringify(operation.variables), replacer}`
      ));
      return null;
    }

Alternatively, have the mock request behave the same as a real world request and strip the undefined values prior to finding the mock.

@pleunv
Copy link
Contributor

pleunv commented Aug 7, 2020

Ran into this just now as well when migrating from 2 to 3, a large part of our apollo tests suddenly started failing and I just couldn't figure out what was going wrong. I think a rollback to the previous behavior (stripping undefined) would probably be desirable as right now it is a massive pain to debug and fix.

@vedrani
Copy link
Contributor

vedrani commented Aug 10, 2020

The problem is because MockLink is not using fast-json-stable-stringify to stringify variables and compare it.

Here is v2 MockLink equality check

Here is v3 MockLink equality check

fast-json-stable-stringify basically strips undefined values.

I'm interested is this a bug in v3 or intentional change?
I cannot find breaking change in Changelog for MockedProvider or MockLink.

@denieler
Copy link

it looks like I also have this issue ☝️

@stephenh
Copy link

stephenh commented Aug 23, 2020

@benjamn looks like MockLink is using your new @wry/equality package but it's a regression for a) not stripping undefined keys (kudos vedrani ^) and also b) not .toJSON-ifying both sides (i.e. a feature of apollo-client is that you can pass in variables that aren't strictly JSON, but it will .toJSON them, i.e. during something like class MyDateUserType into "2020-08-23" by implicitly calling MyDateUserType.toJSON during the previous fast-json-stable-stringify).

benjamn added a commit that referenced this issue Oct 1, 2020
See benjamn/wryware#21 for explanation and
motivation. Should help with #6771, #6803, #6688, #6378, and #7081, and
possibly other regressions that began in @apollo/[email protected].
benjamn added a commit that referenced this issue Oct 1, 2020
See benjamn/wryware#21 for explanation and
motivation. Should help with #6771, #6803, #6688, #6378, and #7081, and
possibly other regressions that began in @apollo/[email protected].
@benjamn
Copy link
Member

benjamn commented Oct 1, 2020

This behavior should be fixed/improved in @apollo/[email protected] (just published to npm), thanks to #7108. Please give it another try after updating!

@benjamn benjamn self-assigned this Oct 1, 2020
@willrax
Copy link
Author

willrax commented Oct 1, 2020

Thanks! I'll update and try it out.

@mrogelja
Copy link

mrogelja commented Dec 4, 2020

Tell me is I'm wrong but this regression seems to remain in @apollo/[email protected] ?

@hwillson
Copy link
Member

hwillson commented May 4, 2021

Let us know if this is still a concern with @apollo/client@latest - thanks!

@hwillson hwillson closed this as completed May 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 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 a pull request may close this issue.

8 participants