Skip to content

Commit

Permalink
debounce schema change events to fix codegen bugs (#3647)
Browse files Browse the repository at this point in the history 
* debounce schema change events to fix codegen bugs

on mass file changes, network schema is requesting schema way too
frequently because the schema cache is invalidated on every schema file
change

to address this, we debounce the onSchemaChange event by 400ms

also, fix bugs with tests, and schemaCacheTTL setting not being passed
to the cache

* changeset
* fix: docs update
* fix: upgrade extension bundlers
* Apply formatting suggestions from code review

thanks @TallTed!

Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
  • Loading branch information
@acao @TallTed
acao and TallTed authored Jul 9, 2024
1 parent e33af28 commit ba5720b
Show file tree
Hide file tree
Showing 11 changed files with 582 additions and 90 deletions.
20 changes: 20 additions & 0 deletions .changeset/eighty-maps-change.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
'graphql-language-service-server': patch
'graphql-language-service-cli': patch
'vscode-graphql': patch
---

**Bugfixes**

debounce schema change events to fix codegen bugs to fix #3622

on mass file changes, network schema is overfetching because the schema cache is now invalidated on every watched schema file change

to address this, we debounce the new `onSchemaChange` event by 400ms

note that `schemaCacheTTL` can only be set in extension settings or graphql config at the top level - it will be ignored if configured per-project in the graphql config

**Code Improvements**

- Fixes flaky tests, and `schemaCacheTTL` setting not being passed to the cache
- Adds a test to validate network schema changes are reflected in the cache
19 changes: 11 additions & 8 deletions packages/graphql-language-service-server/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ module.exports = {
// a function that returns an array of validation rules, ala https://github.com/graphql/graphql-js/tree/main/src/validation/rules
// note that this file will be loaded by the vscode runtime, so the node version and other factors will come into play
customValidationRules: require('./config/customValidationRules'),
schemaCacheTTL: 1000, // reduce or increase minimum schema cache lifetime from 30000ms (30 seconds). you may want to reduce this if you are developing fullstack with network schema
languageService: {
// this is enabled by default if non-local files are specified in the project `schema`
// NOTE: this will disable all definition lookup for local SDL files
Expand Down Expand Up @@ -257,14 +258,16 @@ via `initializationOptions` in nvim.coc. The options are mostly designed to
configure graphql-config's load parameters, the only thing we can't configure
with graphql config. The final option can be set in `graphql-config` as well

| Parameter | Default | Description |
| ----------------------------------------- | ------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `graphql-config.load.baseDir` | workspace root or process.cwd() | the path where graphql config looks for config files |
| `graphql-config.load.filePath` | `null` | exact filepath of the config file. |
| `graphql-config.load.configName` | `graphql` | config name prefix instead of `graphql` |
| `graphql-config.load.legacy` | `true` | backwards compatibility with `graphql-config@2` |
| `graphql-config.dotEnvPath` | `null` | backwards compatibility with `graphql-config@2` |
| `vscode-graphql.cacheSchemaFileForLookup` | `true` if `schema` contains non-sdl files or urls | generate an SDL file based on your graphql-config schema configuration for schema definition lookup and other features. enabled by default when your `schema` config are urls or introspection json, or if you have any non-local SDL files in `schema` |
| Parameter | Default | Description |
| ----------------------------------------- | ------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `graphql-config.load.baseDir` | workspace root or process.cwd() | the path where graphql config looks for config files |
| `graphql-config.load.filePath` | `null` | exact filepath of the config file. |
| `graphql-config.load.configName` | `graphql` | config name prefix instead of `graphql` |
| `graphql-config.load.legacy` | `true` | backwards compatibility with `graphql-config@2` |
| `graphql-config.dotEnvPath` | `null` | backwards compatibility with `graphql-config@2` |
| `vscode-graphql.cacheSchemaFileForLookup` | `true` if `schema` contains non-SDL files or URLs | generate an SDL file based on your graphql-config schema configuration for definition lookup and other features. enabled by default when your `schema` config are URLs or introspection JSON, or if you have any non-local SDL files in `schema` |
| `vscode-graphql.schemaCacheTTL` | `30000` | an integer value in milliseconds for the desired minimum cache lifetime for all schemas, which also causes the generated file to be re-written. set to 30s by default. effectively a "lazy" form of polling. if you are developing a schema alongside client queries, you may want to decrease this |
| `vscode-graphql.debug` | `false` | show more verbose log output in the output channel |

all the `graphql-config.load.*` configuration values come from static
`loadConfig()` options in graphql config.
Expand Down
1 change: 1 addition & 0 deletions packages/graphql-language-service-server/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
"@types/mkdirp": "^1.0.1",
"@types/mock-fs": "^4.13.4",
"cross-env": "^7.0.2",
"debounce-promise": "^3.1.2",
"graphql": "^16.8.1",
"mock-fs": "^5.2.0"
}
Expand Down
45 changes: 20 additions & 25 deletions packages/graphql-language-service-server/src/GraphQLCache.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import {
} from './constants';
import { NoopLogger, Logger } from './Logger';
import { LRUCache } from 'lru-cache';
// import { is } from '@babel/types';

const codeLoaderConfig: CodeFileLoaderConfig = {
noSilentErrors: false,
Expand Down Expand Up @@ -83,12 +84,14 @@ export async function getGraphQLCache({
loadConfigOptions,
config,
onSchemaChange,
schemaCacheTTL,
}: {
parser: typeof parseDocument;
logger: Logger | NoopLogger;
loadConfigOptions: LoadConfigOptions;
config?: GraphQLConfig;
onSchemaChange?: OnSchemaChange;
schemaCacheTTL?: number;
}): Promise<GraphQLCache> {
const graphQLConfig =
config ||
Expand All @@ -105,6 +108,10 @@ export async function getGraphQLCache({
parser,
logger,
onSchemaChange,
schemaCacheTTL:
schemaCacheTTL ??
// @ts-expect-error TODO: add types for extension configs
config?.extensions?.get('languageService')?.schemaCacheTTL,
});
}

Expand All @@ -119,26 +126,29 @@ export class GraphQLCache {
_parser: typeof parseDocument;
_logger: Logger | NoopLogger;
_onSchemaChange?: OnSchemaChange;
_schemaCacheTTL?: number;

constructor({
configDir,
config,
parser,
logger,
onSchemaChange,
schemaCacheTTL,
}: {
configDir: Uri;
config: GraphQLConfig;
parser: typeof parseDocument;
logger: Logger | NoopLogger;
onSchemaChange?: OnSchemaChange;
schemaCacheTTL?: number;
}) {
this._configDir = configDir;
this._graphQLConfig = config;
this._graphQLFileListCache = new Map();
this._schemaMap = new LRUCache({
max: 20,
ttl: 1000 * 30,
ttl: schemaCacheTTL ?? 1000 * 30,
ttlAutopurge: true,
updateAgeOnGet: false,
});
Expand Down Expand Up @@ -602,10 +612,10 @@ export class GraphQLCache {
}

getSchema = async (
projectName: string,
appName?: string,
queryHasExtensions?: boolean | null,
): Promise<GraphQLSchema | null> => {
const projectConfig = this._graphQLConfig.getProject(projectName);
const projectConfig = this._graphQLConfig.getProject(appName);

if (!projectConfig) {
return null;
Expand All @@ -620,35 +630,18 @@ export class GraphQLCache {
if (schemaPath && schemaKey) {
schemaCacheKey = schemaKey as string;

try {
// Read from disk
schema = await projectConfig.loadSchema(
projectConfig.schema,
'GraphQLSchema',
{
assumeValid: true,
assumeValidSDL: true,
experimentalFragmentVariables: true,
sort: false,
includeSources: true,
allowLegacySDLEmptyFields: true,
allowLegacySDLImplementsInterfaces: true,
},
);
} catch {
// // if there is an error reading the schema, just use the last valid schema
schema = this._schemaMap.get(schemaCacheKey);
}

// Maybe use cache
if (this._schemaMap.has(schemaCacheKey)) {
schema = this._schemaMap.get(schemaCacheKey);

if (schema) {
return queryHasExtensions
? this._extendSchema(schema, schemaPath, schemaCacheKey)
: schema;
}
}

// Read from disk
schema = await projectConfig.getSchema();
}

const customDirectives = projectConfig?.extensions?.customDirectives;
Expand All @@ -667,7 +660,9 @@ export class GraphQLCache {

if (schemaCacheKey) {
this._schemaMap.set(schemaCacheKey, schema);
await this._onSchemaChange?.(projectConfig);
if (this._onSchemaChange) {
this._onSchemaChange(projectConfig);
}
}
return schema;
};
Expand Down
8 changes: 6 additions & 2 deletions packages/graphql-language-service-server/src/MessageProcessor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ import { NoopLogger, Logger } from './Logger';
import glob from 'fast-glob';
import { isProjectSDLOnly, unwrapProjectSchema } from './common';
import { DefinitionQueryResponse } from 'graphql-language-service/src/interface';
import { default as debounce } from 'debounce-promise';

const configDocLink =
'https://www.npmjs.com/package/graphql-language-service-server#user-content-graphql-configuration-file';
Expand Down Expand Up @@ -229,7 +230,7 @@ export class MessageProcessor {
rootDir,
};

const onSchemaChange = async (project: GraphQLProjectConfig) => {
const onSchemaChange = debounce(async (project: GraphQLProjectConfig) => {
const { cacheSchemaFileForLookup } =
this.getCachedSchemaSettings(project);
if (!cacheSchemaFileForLookup) {
Expand All @@ -241,7 +242,7 @@ export class MessageProcessor {
return;
}
return this.cacheConfigSchemaFile(project);
};
}, 400);

try {
// now we have the settings so we can re-build the logger
Expand All @@ -255,6 +256,7 @@ export class MessageProcessor {
parser: this._parser,
configDir: rootDir,
onSchemaChange,
schemaCacheTTL: this._settings?.schemaCacheTTL,
});
this._languageService = new GraphQLLanguageService(
this._graphQLCache,
Expand All @@ -267,6 +269,7 @@ export class MessageProcessor {
loadConfigOptions: this._loadConfigOptions,
logger: this._logger,
onSchemaChange,
schemaCacheTTL: this._settings?.schemaCacheTTL,
});
this._languageService = new GraphQLLanguageService(
this._graphQLCache,
Expand Down Expand Up @@ -1307,6 +1310,7 @@ export class MessageProcessor {
unwrapProjectSchema(project).map(async schema => {
const schemaFilePath = path.resolve(project.dirpath, schema);
const uriFilePath = URI.parse(uri).fsPath;

if (uriFilePath === schemaFilePath) {
try {
const file = await readFile(schemaFilePath, 'utf-8');
Expand Down
33 changes: 30 additions & 3 deletions packages/graphql-language-service-server/src/__tests__/MessageProcessor.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,16 @@ import { serializeRange } from './__utils__/utils';
import { readFile } from 'node:fs/promises';
import { existsSync } from 'node:fs';
import { URI } from 'vscode-uri';
import { GraphQLSchema, introspectionFromSchema } from 'graphql';
import {
GraphQLSchema,
buildASTSchema,
introspectionFromSchema,
parse,
} from 'graphql';
import fetchMock from 'fetch-mock';

const sleep = (ms: number) => new Promise(resolve => setTimeout(resolve, ms));

jest.mock('@whatwg-node/fetch', () => {
const { AbortController } = require('node-abort-controller');

Expand All @@ -26,7 +33,7 @@ const mockSchema = (schema: GraphQLSchema) => {
descriptions: true,
}),
};
fetchMock.mock({
return fetchMock.mock({
matcher: '*',
response: {
headers: {
Expand Down Expand Up @@ -523,17 +530,37 @@ describe('MessageProcessor with config', () => {
],
schemaFile,
],
settings: { schemaCacheTTL: 500 },
});

const initParams = await project.init('a/query.ts');
expect(initParams.diagnostics[0].message).toEqual('Unknown fragment "T".');

expect(project.lsp._logger.error).not.toHaveBeenCalled();
expect(await project.lsp._graphQLCache.getSchema('a')).toBeDefined();

fetchMock.restore();
mockSchema(
buildASTSchema(
parse(
'type example100 { string: String } type Query { example: example100 }',
),
),
);
await project.lsp.handleWatchedFilesChangedNotification({
changes: [
{ uri: project.uri('a/fragments.ts'), type: FileChangeType.Changed },
],
});
await sleep(1000);
expect(
(await project.lsp._graphQLCache.getSchema('a')).getType('example100'),
).toBeTruthy();
await sleep(1000);
const file = await readFile(join(genSchemaPath.replace('default', 'a')), {
encoding: 'utf-8',
});
expect(file.split('\n').length).toBeGreaterThan(10);
expect(file).toContain('example100');
// add a new typescript file with empty query to the b project
// and expect autocomplete to only show options for project b
await project.addFile(
Expand Down
8 changes: 6 additions & 2 deletions packages/graphql-language-service-server/src/__tests__/__utils__/MockProject.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export class MockProject {
}: {
files: Files;
root?: string;
settings?: [name: string, vale: any][];
settings?: Record<string, any>;
}) {
this.root = root;
this.fileCache = new Map(files);
Expand Down Expand Up @@ -100,7 +100,11 @@ export class MockProject {
});
}
private mockFiles() {
const mockFiles = { ...defaultMocks };
const mockFiles = {
...defaultMocks,
// without this, the generated schema file may not be cleaned up by previous tests
'/tmp/graphql-language-service': mockfs.directory(),
};
for (const [filename, text] of this.fileCache) {
mockFiles[this.filePath(filename)] = text;
}
Expand Down
2 changes: 2 additions & 0 deletions packages/graphql-language-service/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,13 @@
"graphql": "^15.5.0 || ^16.0.0"
},
"dependencies": {
"debounce-promise": "^3.1.2",
"nullthrows": "^1.0.0",
"vscode-languageserver-types": "^3.17.1"
},
"devDependencies": {
"@types/benchmark": "^1.0.33",
"@types/debounce-promise": "^3.1.9",
"@types/json-schema": "7.0.9",
"@types/picomatch": "^2.3.0",
"benchmark": "^2.1.4",
Expand Down
2 changes: 1 addition & 1 deletion packages/vscode-graphql-syntax/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@
"vscode-oniguruma": "^1.7.0",
"vscode-textmate": "^9.0.0",
"ovsx": "^0.3.0",
"@vscode/vsce": "^2.23.0"
"@vscode/vsce": "^2.22.1-2"
},
"homepage": "https://github.com/graphql/graphiql/blob/main/packages/vscode-graphql-syntax/README.md",
"scripts": {
Expand Down
Loading

0 comments on commit ba5720b

Please sign in to comment.