Fix schema link to return errors for unknown queries #7094
Conversation
| @@ -56,7 +54,9 @@ describe('SchemaLink', () => { | |||
| }); | |||
| observable.subscribe({ | |||
| next, | |||
| error: error => expect(false), | |||
There was a problem hiding this comment.
expect(false) doesn't fail the test. I've replaced this with throwing an error that would really fail the test now.
There was a problem hiding this comment.
Thanks for noticing this and for going out of your way to improve it!
| expect(false); | ||
| done(); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Also this test didn't really assert for anything. The error handler was never called.
| @@ -101,7 +104,9 @@ describe('SchemaLink', () => { | |||
| }); | |||
| observable.subscribe( | |||
| next, | |||
| error => expect(false), | |||
There was a problem hiding this comment.
Same here as mentioned above.
| expect(result.errors![0].message).toMatch(/Cannot query field "unknown"/) | ||
| done(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This is the new test I've added that now passes.
Previously Apollo would return with an empty result object and loading status 7 – "ready". This can lead to unexpected states in apps that use the schema link.
| @@ -56,7 +54,9 @@ describe('SchemaLink', () => { | |||
| }); | |||
| observable.subscribe({ | |||
| next, | |||
| error: error => expect(false), | |||
There was a problem hiding this comment.
Thanks for noticing this and for going out of your way to improve it!
Follow-up to #7094, to avoid enabling validation by default, since @apollo/client@3.3.0 is not a major release.
|
@benjamn I understand, thanks for the hint! Do you have a use case in mind where this would be breaking? The previous behaviour was that an empty object was returned – regardless of what you query. In the apps where I noticed this behaviour, this broke the app, but in more subtle ways (runtime error in components) instead of warning that the query is invalid as soon as it is fired. |
Checklist:
If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changesPlease see the inline comments. I also fixed some tests that I think didn't quite do what they looked like.