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

LSP Rewrite Step 2: refactor for @graphql-tools #3556

Draft
wants to merge 49 commits into
base: recent-missing-coverage
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
65bcc4d
test after coverage for recent features/changes 😬
acao Jan 27, 2024
54658f8
fix schema file cacheing, cache by default
acao Jan 27, 2024
2686b3f
more cleanup, glob if present
acao Jan 27, 2024
9d37093
begin seperating out specs from unit tests
acao Jan 27, 2024
283561e
more unit coverage
acao Jan 27, 2024
8737a89
more unit coverage
acao Jan 27, 2024
d83a7bf
spelling error
acao Jan 27, 2024
a9d0491
test empty cases
acao Jan 28, 2024
d4ede8e
config error handling
acao Feb 1, 2024
7da3b6d
add integration spec coverage for cacheing the schema file!
acao Feb 4, 2024
5ce0741
really exciting spec coverage
acao Feb 4, 2024
12e94f5
more improvements and coverage
acao Feb 4, 2024
0243ae2
refactor the whole integration suite
acao Feb 5, 2024
ee087b2
get set up for a local schema lifecycle
acao Feb 11, 2024
fc04f7c
position job correctly
acao Feb 11, 2024
671df2f
avoid changed schema for now
acao Feb 12, 2024
432e5b9
expose a slightly changed dev server
acao Feb 12, 2024
f74a280
tests not running seperately
acao Feb 12, 2024
b7f905c
fix workflow deps
acao Feb 16, 2024
b5bd0a4
fix eslint
acao Feb 16, 2024
935cb6f
attempt to fix CI only test bug
acao Feb 16, 2024
36242cd
codecov config
acao Feb 16, 2024
8ab3c70
revert test schema change
acao Feb 16, 2024
8913d80
revert config change, restore coverage
acao Feb 18, 2024
bb98420
revert config change, restore coverage
acao Feb 18, 2024
98efb74
cleanup
acao Feb 18, 2024
1a99235
migrate over the wire tests to use local schema instance
acao Feb 18, 2024
7611f2c
test script
acao Feb 18, 2024
c35a156
try to fix this test
acao Feb 18, 2024
128ac4a
fix a few more things related to type cacheing
acao Feb 18, 2024
1910049
fix embedded fragment definition offset bug!
acao Feb 20, 2024
f025e72
spelling bug
acao Feb 20, 2024
0395e64
cleanup
acao Feb 20, 2024
ba7fd81
fix: cleanup, potentially fix project name cache key bug?
acao Feb 27, 2024
0b7115b
fix: delete the unused method
acao Feb 27, 2024
833a727
add comments
acao Feb 27, 2024
525c569
cleanup
acao Feb 27, 2024
cf4536b
feat: lazy initialization on watched file changes
acao Mar 2, 2024
867714c
fix even MORE bugs
acao Mar 3, 2024
9b6304c
fix object field completion, add tests for the missing cases
acao Mar 3, 2024
c1053fd
fix log level, keep things relevant
acao Mar 3, 2024
0036c7f
fix: logger tests, simple re-instantiation on settings change
acao Mar 3, 2024
2d1cbbb
add changeset
acao Mar 3, 2024
479203c
fix: env, timeout
acao Mar 8, 2024
61d91a7
docs: pluck some docs improvements from the next phase
acao Mar 17, 2024
18ab496
fix: refactor for graphql-tools
acao Mar 12, 2024
4216bd6
fix: further ts refinements
acao Mar 17, 2024
310048b
fix: more cleanup
acao Mar 17, 2024
7839f79
only 3 spec errors left!
acao Mar 17, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 35 additions & 0 deletions .changeset/rotten-seahorses-fry.md
@@ -0,0 +1,35 @@
---
'graphql-language-service-server': minor
'vscode-graphql': minor
'graphql-language-service-server-cli': minor
---

Fix many schema and fragment lifecycle issues, for all contexts except for schema updates for url schemas.
Note: this makes `cacheSchemaForLookup` enabled by default again for schema first contexts.

this fixes multiple cacheing bugs, on writing some in-depth integration coverage for the LSP server.
it also solves several bugs regarding loading config types, and properly restarts the server when there are config changes

### Bugfix Summary

- jump to definition in embedded files offset bug
- cache invalidation for fragments
- schema cache invalidation for schema files
- schema definition lookups & autocomplete crossing into the wrong workspace

### Known Bugs Fixed

- #3318
- #2357
- #3469
- #2422
- #2820
- many others to add here...

### Test Improvements

- new, high level integration spec suite for the LSP with a matching test utility
- more unit test coverage
- **total increased test coverage of about 25% in the LSP server codebase.**
- many "happy paths" covered for both schema and code first contexts
- many bugs revealed (and their source)
11 changes: 11 additions & 0 deletions .changeset/silly-yaks-bathe.md
@@ -0,0 +1,11 @@
---
'graphiql': patch
'graphql-language-service': patch
'graphql-language-service-server': patch
'graphql-language-service-server-cli': patch
'codemirror-graphql': patch
'@graphiql/react': patch
'monaco-graphql': patch
---

bugfix to completion for SDL type fields
2 changes: 1 addition & 1 deletion .github/workflows/pr.yml
Expand Up @@ -87,7 +87,7 @@ jobs:
- run: yarn pretty-check

jest:
name: Jest Unit Tests
name: Jest Unit & Integration Tests
runs-on: ubuntu-latest
needs: [install]
steps:
Expand Down
1 change: 1 addition & 0 deletions custom-words.txt
Expand Up @@ -65,6 +65,7 @@ yoshiakis

// packages and tools
argparse
arrayish
astro
astrojs
changesets
Expand Down
1 change: 1 addition & 0 deletions packages/graphiql/test/afterDevServer.js
Expand Up @@ -10,4 +10,5 @@ module.exports = function afterDevServer(_app, _server, _compiler) {
});
// eslint-disable-next-line react-hooks/rules-of-hooks
useServer({ schema }, wsServer);
return wsServer;
};
8 changes: 6 additions & 2 deletions packages/graphiql/test/e2e-server.js
Expand Up @@ -43,7 +43,9 @@ app.post('/graphql-error/graphql', (_req, res, next) => {
app.use(express.static(path.resolve(__dirname, '../')));
app.use('index.html', express.static(path.resolve(__dirname, '../dev.html')));

app.listen(process.env.PORT || 0, function () {
// messy but it allows close
const server = require('node:http').createServer(app);
server.listen(process.env.PORT || 3100, function () {
const { port } = this.address();

console.log(`Started on http://localhost:${port}/`);
Expand All @@ -56,5 +58,7 @@ app.listen(process.env.PORT || 0, function () {
process.exit();
});
});
const wsServer = WebSocketsServer();

WebSocketsServer();
module.exports.server = server;
module.exports.wsServer = wsServer;
18 changes: 9 additions & 9 deletions packages/graphql-language-service-server/README.md
Expand Up @@ -157,7 +157,7 @@ module.exports = {
// 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'),
languageService: {
// should the language service read schema for definition lookups from a cached file based on graphql config output?
// 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
cacheSchemaFileForLookup: true,
// undefined by default which has the same effect as `true`, set to `false` if you are already using // `graphql-eslint` or some other tool for validating graphql in your IDE. Must be explicitly `false` to disable this feature, not just "falsy"
Expand Down Expand Up @@ -237,14 +237,14 @@ 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` | `false` | generate an SDL file based on your graphql-config schema configuration for schema definition lookup and other features. useful when your `schema` config are urls |
| 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` |

all the `graphql-config.load.*` configuration values come from static
`loadConfig()` options in graphql config.
Expand Down
9 changes: 7 additions & 2 deletions packages/graphql-language-service-server/package.json
Expand Up @@ -42,6 +42,10 @@
"@babel/parser": "^7.23.6",
"@babel/types": "^7.23.5",
"@graphql-tools/code-file-loader": "8.0.3",
"@graphql-tools/graphql-tag-pluck": "^8.3.0",
"@graphql-tools/graphql-file-loader": "^8.0.1",
"@graphql-tools/url-loader": "^8.0.2",
"@graphql-tools/utils": "^10.1.2",
"@vue/compiler-sfc": "^3.4.5",
"astrojs-compiler-sync": "^0.3.5",
"cosmiconfig-toml-loader": "^1.0.0",
Expand All @@ -56,15 +60,16 @@
"source-map-js": "1.0.2",
"svelte": "^4.1.1",
"svelte2tsx": "^0.7.0",
"typescript": "^5.3.3",
"vscode-jsonrpc": "^8.0.1",
"vscode-languageserver": "^8.0.1",
"vscode-languageserver-types": "^3.17.2",
"vscode-uri": "^3.0.2",
"typescript": "^5.3.3"
"vscode-uri": "^3.0.2"
},
"devDependencies": {
"@types/glob": "^8.1.0",
"@types/mkdirp": "^1.0.1",
"@types/mock-fs": "^4.13.4",
"cross-env": "^7.0.2",
"graphql": "^16.8.1",
"mock-fs": "^5.2.0"
Expand Down