Improve the document cache miss error message

[orta]

Helps folks get a bit more insight into what's going on when this error is thrown:

[changeset-bot]

🦋 Changeset detected

Latest commit: 714291b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
graphql-language-service-server	Patch
graphql-language-service	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codecov]

Codecov Report

Merging #2161 (714291b) into main (2d91916) will decrease coverage by 0.91%.
The diff coverage is 77.58%.

@@            Coverage Diff             @@
##             main    #2161      +/-   ##
==========================================
- Coverage   65.70%   64.78%   -0.92%     
==========================================
  Files          85       77       -8     
  Lines        5106     5186      +80     
  Branches     1631     1655      +24     
==========================================
+ Hits         3355     3360       +5     
- Misses       1747     1822      +75     
  Partials        4        4              

Impacted Files	Coverage Δ
packages/codemirror-graphql/src/hint.ts	94.73% <ø> (ø)
packages/codemirror-graphql/src/lint.ts	100.00% <ø> (ø)
packages/codemirror-graphql/src/results/mode.ts	47.05% <ø> (ø)
...kages/codemirror-graphql/src/utils/forEachState.ts	100.00% <ø> (ø)
...ckages/codemirror-graphql/src/utils/mode-indent.ts	0.00% <0.00%> (ø)
packages/codemirror-graphql/src/variables/hint.ts	89.70% <ø> (ø)
packages/codemirror-graphql/src/variables/mode.ts	79.48% <ø> (ø)
packages/graphiql/src/components/QueryEditor.tsx	63.96% <ø> (ø)
packages/graphiql/src/utility/fillLeafs.ts	5.33% <ø> (ø)
...kages/graphiql/src/utility/introspectionQueries.ts	100.00% <ø> (ø)
... and 56 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc60ba0...714291b. Read the comment docs.

[acao]

Thanks for this! Added to the pinned roadmap

In most cases this log message doesn't need to be reported at all.

For example, this error is shown when files are parsed but no documents are found. this will show for .js/ts/etc files without embedded graphql. There is some over-parsing going on, which is why you see it so frequently

A more relevant message is clipped at the top, which indicates an error from graphql-config. This and other graphql-config error messages we should show in debug output, so that users can debug with graphql-config maintainers more easily.

Again, thanks a bunch for this! This is a good step in the right direction for error logging in the LSP server

[acao]

@orta can you add a changeset?

[acao]

I can't remember offhand if there's a log level setting - perhaps these can be debug() level output? Or removed. Do you think they are useful? I don't think so, as per the comment

[acao]

Sorry I meant to put this in a review before. Can we just remove these errors and return if !cachedDocument ? My earlier comment explains why. There is a section in the roadmap or somewhere where I discuss removing these

[acao]

So these messages are a great improvement to be sure, but instead of throwing an error, we can do this:

if (!cachedDocument) {
 	this.logger.debug(this._errorMessageForMissingDocument(textDocument.uri));
 	return
}

i would much prefer it, as these messages are mostly not useful

we could potentially warn if there are no files in the cache though. "no files found in document cache, please check your configuration"

we have a bunch of roadmap items about better notifications for graphql config presence & validation that would solve many visibility issues upstream, reducing the need to debug at this level.

also as I point out in the comment, the errors are misleading because often these errors are for files that don't contain graphql or aren't parseable. a cache miss is rarely a bad thing, but generally, if it is happening for files with embedded graphql, it's because of configuration issues

[orta]

Yeah, agree, we should remove the error messages realistically because they're just not that useful

[acao]

This was another one of those situations where I couldn’t understand why it was there in the first place. Previously almost nothing went to the output panel and all logging went to file, but I exposed some to output panel, and the logging to file is yet another thing to question, another artefact of the pre-LSP era of this server

[orta]

Switched out to NOOPs 👍🏻

[acao]

@orta beautiful, thank you!