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

pass along missing field errors to the user #8262

Merged
merged 3 commits into from
May 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@
- Fully remove result cache entries from LRU dependency system when the corresponding entities are removed from `InMemoryCache` by eviction, or by any other means. <br/>
[@sofianhn](https://github.com/sofianhn) and [@benjamn](https://github.com/benjamn) in [#8147](https://github.com/apollographql/apollo-client/pull/8147)

- Expose missing field errors in results. <br/> [@brainkim](github.com/brainkim) in [#8262](https://github.com/apollographql/apollo-client/pull/8262)

### Documentation
TBD

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
{
"name": "apollo-client",
"path": "./dist/apollo-client.cjs.min.js",
"maxSize": "26.6 kB"
"maxSize": "26.62 kB"
}
],
"peerDependencies": {
Expand Down
10 changes: 7 additions & 3 deletions src/cache/core/types/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,18 @@ import { StorageType } from '../../inmemory/policies';
// Readonly<any>, somewhat surprisingly.
export type SafeReadonly<T> = T extends object ? Readonly<T> : T;

export class MissingFieldError {
export class MissingFieldError extends Error {
benjamn marked this conversation as resolved.
Show resolved Hide resolved
constructor(
public readonly message: string,
public readonly path: (string | number)[],
public readonly query: import('graphql').DocumentNode,
public readonly clientOnly: boolean,
public readonly variables?: Record<string, any>,
) {}
) {
super(message);
// We're not using `Object.setPrototypeOf` here as it isn't fully
// supported on Android (see issue #3236).
(this as any).__proto__ = MissingFieldError.prototype;
}
}

export interface FieldSpecifier {
Expand Down
4 changes: 0 additions & 4 deletions src/cache/inmemory/__tests__/entityStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1213,14 +1213,12 @@ describe('EntityStore', () => {
'Can\'t find field \'hobby\' on Author:{"name":"Ted Chiang"} object',
["authorOfBook", "hobby"],
expect.anything(), // query
false, // clientOnly
expect.anything(), // variables
),
new MissingFieldError(
'Can\'t find field \'publisherOfBook\' on ROOT_QUERY object',
["publisherOfBook"],
expect.anything(), // query
false, // clientOnly
expect.anything(), // variables
),
],
Expand Down Expand Up @@ -1814,7 +1812,6 @@ describe('EntityStore', () => {
"Dangling reference to missing Author:2 object",
["book", "author"],
expect.anything(), // query
false, // clientOnly
expect.anything(), // variables
),
];
Expand Down Expand Up @@ -2084,7 +2081,6 @@ describe('EntityStore', () => {
'Can\'t find field \'title\' on Book:{"isbn":"031648637X"} object',
["book", "title"],
expect.anything(), // query
false, // clientOnly
expect.anything(), // variables
),
],
Expand Down
1 change: 0 additions & 1 deletion src/cache/inmemory/__tests__/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1352,7 +1352,6 @@ describe("type policies", function () {
`Can't find field 'result' on Job:{"name":"Job #${jobNumber}"} object`,
["jobs", jobNumber - 1, "result"],
expect.anything(), // query
false, // clientOnly
expect.anything(), // variables
);
}
Expand Down
2 changes: 0 additions & 2 deletions src/cache/inmemory/__tests__/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,6 @@ describe('reading from the store', () => {
}`,
["normal", "missing"],
query,
false, // clientOnly
{}, // variables
),
new MissingFieldError(
Expand All @@ -738,7 +737,6 @@ describe('reading from the store', () => {
}`,
["clientOnly", "missing"],
query,
true, // clientOnly
{}, // variables
),
]);
Expand Down
19 changes: 0 additions & 19 deletions src/cache/inmemory/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ interface ReadContext extends ReadMergeModifyContext {
canonizeResults: boolean;
fragmentMap: FragmentMap;
path: (string | number)[];
clientOnly: boolean;
};

export type ExecResult<R = any> = {
Expand All @@ -62,7 +61,6 @@ function missingFromInvariant(
err.message,
context.path.slice(),
context.query,
context.clientOnly,
context.variables,
);
}
Expand Down Expand Up @@ -241,7 +239,6 @@ export class StoreReader {
canonizeResults,
fragmentMap: createFragmentMap(getFragmentDefinitions(query)),
path: [],
clientOnly: false,
},
});

Expand Down Expand Up @@ -344,20 +341,6 @@ export class StoreReader {
const resultName = resultKeyNameFromField(selection);
context.path.push(resultName);

// If this field has an @client directive, then the field and
// everything beneath it is client-only, meaning it will never be
// sent to the server.
const wasClientOnly = context.clientOnly;
// Once we enter a client-only subtree of the query, we can avoid
// repeatedly checking selection.directives.
context.clientOnly = wasClientOnly || !!(
// We don't use the hasDirectives helper here, because it looks
// for directives anywhere inside the AST node, whereas we only
// care about directives directly attached to this field.
selection.directives &&
selection.directives.some(d => d.name.value === "client")
);

if (fieldValue === void 0) {
if (!addTypenameToDocument.added(selection)) {
getMissing().push(
Expand Down Expand Up @@ -407,8 +390,6 @@ export class StoreReader {
objectsToMerge.push({ [resultName]: fieldValue });
}

context.clientOnly = wasClientOnly;

invariant(context.path.pop() === resultName);

} else {
Expand Down
11 changes: 11 additions & 0 deletions src/core/ObservableQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ export class ObservableQuery<
!this.queryManager.transform(this.options.query).hasForcedResolvers
) {
const diff = this.queryInfo.getDiff();
// XXX the only reason this typechecks is that diff.result is inferred as any
result.data = (
diff.complete ||
this.options.returnPartialData
Expand All @@ -180,6 +181,16 @@ export class ObservableQuery<
} else {
result.partial = true;
}

if (
!diff.complete &&
!this.options.partialRefetch &&
!result.loading &&
!result.data &&
!result.error
) {
result.error = new ApolloError({ clientErrors: diff.missing });
}
}

if (saveAsLastResult) {
Expand Down
14 changes: 10 additions & 4 deletions src/errors/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@ export function isApolloError(err: Error): err is ApolloError {
const generateErrorMessage = (err: ApolloError) => {
let message = '';
// If we have GraphQL errors present, add that to the error message.
if (isNonEmptyArray(err.graphQLErrors)) {
err.graphQLErrors.forEach((graphQLError: GraphQLError) => {
const errorMessage = graphQLError
? graphQLError.message
if (isNonEmptyArray(err.graphQLErrors) || isNonEmptyArray(err.clientErrors)) {
const errors = ((err.graphQLErrors || []) as readonly Error[])
.concat(err.clientErrors || []);
errors.forEach((error: Error) => {
const errorMessage = error
? error.message
: 'Error message not found.';
message += `${errorMessage}\n`;
});
Expand All @@ -36,6 +38,7 @@ const generateErrorMessage = (err: ApolloError) => {
export class ApolloError extends Error {
public message: string;
public graphQLErrors: ReadonlyArray<GraphQLError>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I consider Readonly to be a “bad part” of TypeScript but I’m not gonna rock the boat here.

public clientErrors: ReadonlyArray<Error>;
public networkError: Error | ServerParseError | ServerError | null;

// An object that can be used to provide some additional information
Expand All @@ -48,17 +51,20 @@ export class ApolloError extends Error {
// value or the constructed error will be meaningless.
constructor({
graphQLErrors,
clientErrors,
networkError,
errorMessage,
extraInfo,
}: {
graphQLErrors?: ReadonlyArray<GraphQLError>;
clientErrors?: ReadonlyArray<Error>;
networkError?: Error | ServerParseError | ServerError | null;
errorMessage?: string;
extraInfo?: any;
}) {
super(errorMessage);
this.graphQLErrors = graphQLErrors || [];
this.clientErrors = clientErrors || [];
this.networkError = networkError || null;
this.message = errorMessage || generateErrorMessage(this);
this.extraInfo = extraInfo;
Expand Down
77 changes: 77 additions & 0 deletions src/react/hooks/__tests__/useQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2645,6 +2645,83 @@ describe('useQuery Hook', () => {
});
});

describe('Missing Fields', () => {
itAsync(
'should have errors populated with missing field errors from the cache',
(resolve, reject) => {
const carQuery: DocumentNode = gql`
query cars($id: Int) {
cars(id: $id) {
id
make
model
vin
__typename
}
}
`;

const carData = {
cars: [
{
id: 1,
make: 'Audi',
model: 'RS8',
vine: 'DOLLADOLLABILL',
__typename: 'Car'
}
]
};

const mocks = [
{
request: { query: carQuery, variables: { id: 1 } },
result: { data: carData }
},
];

let renderCount = 0;
function App() {
const { loading, data, error } = useQuery(carQuery, {
variables: { id: 1 },
});

switch (renderCount) {
case 0:
expect(loading).toBeTruthy();
expect(data).toBeUndefined();
expect(error).toBeUndefined();
break;
case 1:
expect(loading).toBeFalsy();
expect(data).toBeUndefined();
expect(error).toBeDefined();
// TODO: ApolloError.name is Error for some reason
// expect(error!.name).toBe(ApolloError);
expect(error!.clientErrors.length).toEqual(1);
expect(error!.message).toMatch(/Can't find field 'vin' on Car:1/);
break;
default:
throw new Error("Unexpected render");
}

renderCount += 1;
return null;
}

render(
<MockedProvider mocks={mocks}>
<App />
</MockedProvider>
);

return wait(() => {
expect(renderCount).toBe(2);
}).then(resolve, reject);
},
);
});

describe('Previous data', () => {
itAsync('should persist previous data when a query is re-run', (resolve, reject) => {
const query = gql`
Expand Down