-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Improve the document cache miss error message #2161
Conversation
🦋 Changeset detectedLatest commit: 714291b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 Report
@@ 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
Continue to review full report at Codecov.
|
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 Again, thanks a bunch for this! This is a good step in the right direction for error logging in the LSP server |
@orta can you add a changeset? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -485,7 +485,7 @@ export class MessageProcessor { | |||
|
|||
const cachedDocument = this._getCachedDocument(textDocument.uri); | |||
if (!cachedDocument) { | |||
throw new Error('A cached document cannot be found.'); | |||
throw new Error(this._errorMessageForMissingDocument(textDocument.uri)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 |
Yeah, agree, we should remove the error messages realistically because they're just not that useful |
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 |
Switched out to NOOPs 👍🏻 |
@orta beautiful, thank you! |
Helps folks get a bit more insight into what's going on when this error is thrown: