-
Notifications
You must be signed in to change notification settings - Fork 113
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
create links for values of format: uri-reference #219
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
ping @aeschli |
src/services/jsonLinks.ts
Outdated
@@ -6,24 +6,75 @@ | |||
import { DocumentLink } from 'vscode-languageserver-types'; | |||
import { TextDocument, ASTNode, PropertyASTNode, Range, Thenable } from '../jsonLanguageTypes'; | |||
import { JSONDocument } from '../parser/jsonParser'; | |||
import { IJSONSchemaService } from './jsonSchemaService'; | |||
import { URI } from 'vscode-uri'; | |||
import { existsSync as fileExistsSync } from 'fs'; |
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.
We want to run that code also in the browser, so we can not use NodeJS-API like fs
.
Instead we need to provide file system access through an abstraction.
We already have that in the css-languageservice: https://github.com/microsoft/vscode-css-languageservice/blob/3e2d7992179c81dc327c5e1ec8b2ec59cb2220f1/src/cssLanguageTypes.ts#L285
Can you follow that example?
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.
Sure! If I'm understanding things correctly, it looks like I'll need to use stat
(I didn't see an exists function in the fs provider docs) and handle the promise it returns.
Then when I eventually want to use this in the yaml language server, I'm assuming I'll need to do something similar to what you did here.
Please let me know if I'm not on right path.
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.
Looks like my link was corrupted, I updated it.
What's to do:
- add new type to jsonLanguageTypes.ts:
export interface FileSystemProvider {
stat(uri: DocumentUri): Promise<FileStat>;
}
plus FileStat
and FileType
(copy these from the cssLanguageTypes)
- add a new property
/**
* Abstract file system access away from the service.
* Used for dynamic link resolving, etc.
*/
fileSystemProvider?: FileSystemProvider;
to LanguageServiceParams
- pass that to your code and use it, if available (all is optional, we don't want to break existing clients)
- document these types in the CHANGELOG.md
- in followup PRs we have to update the consumers of the
vscode-json-languageservice
(yaml-extension and json-language-feature) to provide a FileSystemProvider implementation. I can do that forvscode-json-languageservice
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.
@aeschli Thanks for the detailed breakdown - made things much easier! I rebased against main and made the suggested changes.
I don't do much JavaScript development, and I'm not 100% happy w/ how complicated all the promise logic ended up where I'm checking if the file exists. If you have any suggestions for simplifying things, I'm all ears 😄.
2650568
to
c5705dd
Compare
} | ||
|
||
export interface FileSystemProvider { | ||
stat(uri: DocumentUri): Promise<FileStat>; |
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.
Please add specs here too, in particular what happens when the file does not exist.
This PR adds logic to
findLinks
to create links for relative path names when the associated JSON schema is configured withformat: uri-reference
.Fixes: microsoft/vscode#128971
Unblocks: redhat-developer/vscode-yaml#559
A few notes:
I tried to make the change as backwards compatible as possible by adding a new, optional
schemaService
param tofindLinks
.This was important because the YAML language server delegates to
findLinks
, and I'm primarily interested in getting this functionality working over there.I assumed that the
fileExists
check would be a requirement (i.e. links for non existent files would be a poor UX). It's only being ran for properties w/format: uri-reference
, so it shouldn't be much overhead, but I'm happy to remove if you disagree.