-
Notifications
You must be signed in to change notification settings - Fork 468
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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
- Loading branch information
1 parent
3a6b11a
commit c8f2e92
Showing
22 changed files
with
473 additions
and
126 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { fs } from "memfs"; | ||
|
|
||
| module.exports = fs; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
220 changes: 220 additions & 0 deletions
220
packages/apollo-language-server/src/errors/__tests__/NoMissingClientDirectives.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,220 @@ | ||
| import { NoMissingClientDirectives } from "../validation"; | ||
| import { GraphQLClientProject } from "../../project/client"; | ||
| import { readFileSync } from "fs"; | ||
| import { basename } from "path"; | ||
|
|
||
| import { vol } from "memfs"; | ||
| import { LoadingHandler } from "../../loadingHandler"; | ||
| import { ApolloConfig, ClientConfig } from "../../config"; | ||
| import URI from "vscode-uri"; | ||
|
|
||
| const serviceSchema = /* GraphQL */ ` | ||
| type Query { | ||
| me: User | ||
| } | ||
| type User { | ||
| name: String | ||
| friends: [User] | ||
| } | ||
| `; | ||
| const clientSchema = /* GraphQL */ ` | ||
| extend type Query { | ||
| isOnline: Boolean | ||
| } | ||
| extend type User { | ||
| isLiked: Boolean | ||
| localUser: User | ||
| } | ||
| `; | ||
| const a = /* GraphQL */ ` | ||
| query a { | ||
| isOnline | ||
| me { | ||
| name | ||
| localUser @client { | ||
| friends { | ||
| isLiked | ||
| } | ||
| } | ||
| friends { | ||
| name | ||
| isLiked | ||
| } | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const b = /* GraphQL */ ` | ||
| query b { | ||
| me { | ||
| ... { | ||
| isLiked | ||
| } | ||
| ... @client { | ||
| localUser { | ||
| name | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const c = /* GraphQL */ ` | ||
| query c { | ||
| me { | ||
| ...isLiked | ||
| } | ||
| } | ||
| fragment localUser on User @client { | ||
| localUser { | ||
| name | ||
| } | ||
| } | ||
| fragment isLiked on User { | ||
| isLiked | ||
| ...localUser | ||
| } | ||
| `; | ||
|
|
||
| const d = /* GraphQL */ ` | ||
| fragment isLiked on User { | ||
| isLiked | ||
| } | ||
| query d { | ||
| me { | ||
| ...isLiked | ||
| ...locaUser | ||
| } | ||
| } | ||
| fragment localUser on User @client { | ||
| localUser { | ||
| name | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const e = /* GraphQL */ ` | ||
| fragment friends on User { | ||
| friends { | ||
| ...isLiked | ||
| ... on User @client { | ||
| localUser { | ||
| name | ||
| } | ||
| } | ||
| } | ||
| } | ||
| query e { | ||
| isOnline @client | ||
| me { | ||
| ...friends | ||
| } | ||
| } | ||
| fragment isLiked on User { | ||
| isLiked | ||
| } | ||
| `; | ||
|
|
||
| // TODO support inline fragment spreads | ||
| const f = /* GraphQL */ ` | ||
| query f { | ||
| me { | ||
| ...isLiked @client | ||
| } | ||
| } | ||
| fragment isLiked on User { | ||
| isLiked | ||
| } | ||
| `; | ||
|
|
||
| const rootURI = URI.file(process.cwd()); | ||
|
|
||
| const config = new ApolloConfig({ | ||
| client: { | ||
| service: { | ||
| name: "server", | ||
| localSchemaFile: "./schema.graphql" | ||
| }, | ||
| includes: ["./src/**.graphql"], | ||
| excludes: ["./__tests__"], | ||
| validationRules: [NoMissingClientDirectives] | ||
| }, | ||
| engine: {} | ||
| }) as ClientConfig; | ||
|
|
||
| class MockLoadingHandler implements LoadingHandler { | ||
| handle<T>(_message: string, value: Promise<T>): Promise<T> { | ||
| return value; | ||
| } | ||
| handleSync<T>(_message: string, value: () => T): T { | ||
| return value(); | ||
| } | ||
| showError(_message: string): void {} | ||
| } | ||
|
|
||
| jest.mock("fs"); | ||
|
|
||
| describe("client state", () => { | ||
| beforeEach(() => { | ||
| vol.fromJSON({ | ||
| "apollo.config.js": `module.exports = { | ||
| client: { | ||
| service: { | ||
| localSchemaFile: './schema.graphql' | ||
| } | ||
| } | ||
| }`, | ||
| "schema.graphql": serviceSchema, | ||
| "src/client-schema.graphql": clientSchema, | ||
| "src/a.graphql": a, | ||
| "src/b.graphql": b, | ||
| "src/c.graphql": c, | ||
| "src/d.graphql": d, | ||
| "src/e.graphql": e | ||
| // "src/f.graphql": f, | ||
| }); | ||
| }); | ||
| afterEach(jest.restoreAllMocks); | ||
|
|
||
| it("should report validation errors for missing @client directives", async () => { | ||
| const project = new GraphQLClientProject({ | ||
| config, | ||
| loadingHandler: new MockLoadingHandler(), | ||
| rootURI | ||
| }); | ||
|
|
||
| const errors = Object.create(null); | ||
| project.onDiagnostics(({ diagnostics, uri }) => { | ||
| const path = basename(URI.parse(uri).path); | ||
| diagnostics.forEach(({ error }: any) => { | ||
| if (!errors[path]) errors[path] = []; | ||
| errors[path].push(error); | ||
| }); | ||
| }); | ||
|
|
||
| await project.whenReady; | ||
| await project.validate(); | ||
|
|
||
| expect(errors).toMatchInlineSnapshot(` | ||
| Object { | ||
| "a.graphql": Array [ | ||
| [GraphQLError: @client directive is missing on local field "isOnline"], | ||
| [GraphQLError: @client directive is missing on local field "isLiked"], | ||
| ], | ||
| "b.graphql": Array [ | ||
| [GraphQLError: @client directive is missing on fragment around local fields "isLiked"], | ||
| ], | ||
| "c.graphql": Array [ | ||
| [GraphQLError: @client directive is missing on fragment "isLiked" around local fields "isLiked,localUser"], | ||
| ], | ||
| "d.graphql": Array [ | ||
| [GraphQLError: @client directive is missing on fragment "isLiked" around local fields "isLiked"], | ||
| ], | ||
| "e.graphql": Array [ | ||
| [GraphQLError: @client directive is missing on fragment "isLiked" around local fields "isLiked"], | ||
| ], | ||
| } | ||
| `); | ||
| }); | ||
| }); |
Oops, something went wrong.