Skip to content

Commit c8f2e92

Browse files
James BaxleyJakeDawkins
authored andcommitted
Improve performance of client directive validation (#1559)
* This commit seeks to improve the performance when determining if a field is missing any @client directives when using local state with Apollo Client. Instead of recursing over the entire document for each field, we walk the tree once while checking the schema for any possible client side annotations. This PR is an initial take at improving this workload but doesn't complete the work. Still to be done includes supporting @client validation on fragment spreads * fix TS error on failure to clean up removed function * Improve startup performance of CLI by removing extra validation Historically, the language server was designed to be used just with the VS Code extension (and other editors) where, after loading the project, the first thing we want to do is validate all of the documents and schema. However, we resuse the base projects (client / server) in the CLI to share common work especially around document management. Due to this, when the CLI starts up, its runs a potentially expensive validation step regardless of if the command has anything to do with validation. Often this is a no-op because of a missing schema, but given the correct env variables that loading can still happen automatically (something to clean up later). This change moves the logic of "auto validate" to the concept of a workspace which is used in long running processes (currently the VS Code but soon the CLI watch mode) and keeps the base project class clean. It also removes the hack in place that helped to highlight this in the recent added test suite for client validation. * update changelog * Pragmatic take on improving codegen efficiency This simplifies the watch and non watch modes by only calling write within the watcher and not relying on the underyling documentChange events from the project base. It also add a *VERY* simple exclude support which is far from the right solution but prevents more loops then currently happen * Add gitContext to checkPartialSchema mutation (#1560) * add gitContext to federated checkPartialSchema mutation * Clarify changelog and fix typo
1 parent 3a6b11a commit c8f2e92

22 files changed

Lines changed: 473 additions & 126 deletions

File tree

CHANGELOG.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,12 @@
33
## Upcoming
44

55
- `apollo`
6+
- Improve performance of CLI when running projects by delaying or not calling validation unnecessarily [#1559](https://github.com/apollographql/apollo-tooling/pull/1559)
67
- Use "table" package for tabular output and word wrap support [#1524](https://github.com/apollographql/apollo-tooling/pull/1524)
78
- Use new single step mutation for checking federated service schemas [#1539](https://github.com/apollographql/apollo-tooling/pull/1539)
89
- Add support for `localSchemaFile` for federated service commands [#1489](https://github.com/apollographql/apollo-tooling/pull/1489)
910
- `apollo-codegen-core`
10-
- <First `apollo-codegen-core` related entry goes here>
11+
- Improve performance of validation when client fields are present or not [#1559](https://github.com/apollographql/apollo-tooling/pull/1559)
1112
- `apollo-codegen-flow`
1213
- <First `apollo-codegen-flow` related entry goes here>
1314
- `apollo-codegen-scala`
@@ -28,7 +29,7 @@
2829
- `apollo-tools`
2930
- <First `apollo-tools` related entry goes here>
3031
- `vscode-apollo`
31-
- <First `vscode-apollo` related entry goes here>
32+
- Improve performance of validation when client fields are present or not [#1559](https://github.com/apollographql/apollo-tooling/pull/1559)
3233

3334
## `apollo@2.18.3`
3435

packages/apollo-codegen-core/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "apollo-codegen-core",
33
"description": "Core generator APIs for Apollo Codegen",
4-
"version": "0.36.0-alpha.1",
4+
"version": "0.36.0-alpha.2",
55
"author": "Apollo GraphQL <opensource@apollographql.com>",
66
"license": "MIT",
77
"repository": {

packages/apollo-codegen-flow/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "apollo-codegen-flow",
33
"description": "Flow generator module for Apollo Codegen",
4-
"version": "0.34.0-alpha.1",
4+
"version": "0.34.0-alpha.2",
55
"author": "Apollo GraphQL <opensource@apollographql.com>",
66
"license": "MIT",
77
"repository": {

packages/apollo-codegen-scala/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "apollo-codegen-scala",
33
"description": "Scala generator module for Apollo Codegen",
4-
"version": "0.35.0-alpha.1",
4+
"version": "0.35.0-alpha.2",
55
"author": "Apollo GraphQL <opensource@apollographql.com>",
66
"license": "MIT",
77
"repository": {

packages/apollo-codegen-swift/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "apollo-codegen-swift",
33
"description": "Swift generator module for Apollo Codegen",
4-
"version": "0.36.0-alpha.1",
4+
"version": "0.36.0-alpha.2",
55
"author": "Apollo GraphQL <opensource@apollographql.com>",
66
"license": "MIT",
77
"repository": {

packages/apollo-codegen-typescript/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "apollo-codegen-typescript",
33
"description": "TypeScript generator module for Apollo Codegen",
4-
"version": "0.36.0-alpha.1",
4+
"version": "0.36.0-alpha.2",
55
"author": "Apollo GraphQL <opensource@apollographql.com>",
66
"license": "MIT",
77
"repository": {

packages/apollo-graphql/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "apollo-graphql",
3-
"version": "0.3.4-alpha.0",
3+
"version": "0.3.4-alpha.1",
44
"description": "Apollo GraphQL utility library",
55
"main": "lib/index.js",
66
"types": "lib/index.d.ts",
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
import { fs } from "memfs";
2+
3+
module.exports = fs;

packages/apollo-language-server/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "apollo-language-server",
33
"description": "A language server for Apollo GraphQL projects",
4-
"version": "1.16.0-alpha.1",
4+
"version": "1.16.0-alpha.2",
55
"author": "Apollo GraphQL <opensource@apollographql.com>",
66
"license": "MIT",
77
"repository": {
Lines changed: 220 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,220 @@
1+
import { NoMissingClientDirectives } from "../validation";
2+
import { GraphQLClientProject } from "../../project/client";
3+
import { readFileSync } from "fs";
4+
import { basename } from "path";
5+
6+
import { vol } from "memfs";
7+
import { LoadingHandler } from "../../loadingHandler";
8+
import { ApolloConfig, ClientConfig } from "../../config";
9+
import URI from "vscode-uri";
10+
11+
const serviceSchema = /* GraphQL */ `
12+
type Query {
13+
me: User
14+
}
15+
16+
type User {
17+
name: String
18+
friends: [User]
19+
}
20+
`;
21+
const clientSchema = /* GraphQL */ `
22+
extend type Query {
23+
isOnline: Boolean
24+
}
25+
extend type User {
26+
isLiked: Boolean
27+
localUser: User
28+
}
29+
`;
30+
const a = /* GraphQL */ `
31+
query a {
32+
isOnline
33+
me {
34+
name
35+
localUser @client {
36+
friends {
37+
isLiked
38+
}
39+
}
40+
friends {
41+
name
42+
isLiked
43+
}
44+
}
45+
}
46+
`;
47+
48+
const b = /* GraphQL */ `
49+
query b {
50+
me {
51+
... {
52+
isLiked
53+
}
54+
... @client {
55+
localUser {
56+
name
57+
}
58+
}
59+
}
60+
}
61+
`;
62+
63+
const c = /* GraphQL */ `
64+
query c {
65+
me {
66+
...isLiked
67+
}
68+
}
69+
fragment localUser on User @client {
70+
localUser {
71+
name
72+
}
73+
}
74+
fragment isLiked on User {
75+
isLiked
76+
...localUser
77+
}
78+
`;
79+
80+
const d = /* GraphQL */ `
81+
fragment isLiked on User {
82+
isLiked
83+
}
84+
query d {
85+
me {
86+
...isLiked
87+
...locaUser
88+
}
89+
}
90+
fragment localUser on User @client {
91+
localUser {
92+
name
93+
}
94+
}
95+
`;
96+
97+
const e = /* GraphQL */ `
98+
fragment friends on User {
99+
friends {
100+
...isLiked
101+
... on User @client {
102+
localUser {
103+
name
104+
}
105+
}
106+
}
107+
}
108+
query e {
109+
isOnline @client
110+
me {
111+
...friends
112+
}
113+
}
114+
fragment isLiked on User {
115+
isLiked
116+
}
117+
`;
118+
119+
// TODO support inline fragment spreads
120+
const f = /* GraphQL */ `
121+
query f {
122+
me {
123+
...isLiked @client
124+
}
125+
}
126+
fragment isLiked on User {
127+
isLiked
128+
}
129+
`;
130+
131+
const rootURI = URI.file(process.cwd());
132+
133+
const config = new ApolloConfig({
134+
client: {
135+
service: {
136+
name: "server",
137+
localSchemaFile: "./schema.graphql"
138+
},
139+
includes: ["./src/**.graphql"],
140+
excludes: ["./__tests__"],
141+
validationRules: [NoMissingClientDirectives]
142+
},
143+
engine: {}
144+
}) as ClientConfig;
145+
146+
class MockLoadingHandler implements LoadingHandler {
147+
handle<T>(_message: string, value: Promise<T>): Promise<T> {
148+
return value;
149+
}
150+
handleSync<T>(_message: string, value: () => T): T {
151+
return value();
152+
}
153+
showError(_message: string): void {}
154+
}
155+
156+
jest.mock("fs");
157+
158+
describe("client state", () => {
159+
beforeEach(() => {
160+
vol.fromJSON({
161+
"apollo.config.js": `module.exports = {
162+
client: {
163+
service: {
164+
localSchemaFile: './schema.graphql'
165+
}
166+
}
167+
}`,
168+
"schema.graphql": serviceSchema,
169+
"src/client-schema.graphql": clientSchema,
170+
"src/a.graphql": a,
171+
"src/b.graphql": b,
172+
"src/c.graphql": c,
173+
"src/d.graphql": d,
174+
"src/e.graphql": e
175+
// "src/f.graphql": f,
176+
});
177+
});
178+
afterEach(jest.restoreAllMocks);
179+
180+
it("should report validation errors for missing @client directives", async () => {
181+
const project = new GraphQLClientProject({
182+
config,
183+
loadingHandler: new MockLoadingHandler(),
184+
rootURI
185+
});
186+
187+
const errors = Object.create(null);
188+
project.onDiagnostics(({ diagnostics, uri }) => {
189+
const path = basename(URI.parse(uri).path);
190+
diagnostics.forEach(({ error }: any) => {
191+
if (!errors[path]) errors[path] = [];
192+
errors[path].push(error);
193+
});
194+
});
195+
196+
await project.whenReady;
197+
await project.validate();
198+
199+
expect(errors).toMatchInlineSnapshot(`
200+
Object {
201+
"a.graphql": Array [
202+
[GraphQLError: @client directive is missing on local field "isOnline"],
203+
[GraphQLError: @client directive is missing on local field "isLiked"],
204+
],
205+
"b.graphql": Array [
206+
[GraphQLError: @client directive is missing on fragment around local fields "isLiked"],
207+
],
208+
"c.graphql": Array [
209+
[GraphQLError: @client directive is missing on fragment "isLiked" around local fields "isLiked,localUser"],
210+
],
211+
"d.graphql": Array [
212+
[GraphQLError: @client directive is missing on fragment "isLiked" around local fields "isLiked"],
213+
],
214+
"e.graphql": Array [
215+
[GraphQLError: @client directive is missing on fragment "isLiked" around local fields "isLiked"],
216+
],
217+
}
218+
`);
219+
});
220+
});

0 commit comments

Comments
 (0)