-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add editor support for JSON config schema #4724
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
Add editor support for JSON config schema #4724
Conversation
f99e430
to
82ea067
Compare
e7e0561
to
eb29575
Compare
65293b7
to
29ede39
Compare
I just had another idea. Since the VSCode extension already knows how to invoke the compiler binary, and the compiler binary can construct the json schema, what if we added a separate compiler command to return the schema text on stdout? It seems like that might be simpler. It would also work better for us internally since we don't access the compiler via an NPM module but a separate binary deployment which would be hard to update to also sync a sibling file. I'll work on a compile diff to add this support. |
9c31623
to
0982908
Compare
|
||
provideTextDocumentContent(uri: Uri): ProviderResult<string> { | ||
if (uri.authority === PACKAGE_JSON_RELAY_CONFIG_SCHEMA_PATH) { | ||
return `{"properties": { "relay": { "$ref": "${RelayTextDocumentContentProvider.scheme}://${RELAY_CONFIG_SCHEMA_PATH}", "description": "Relay.js configuration" }}}`; |
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.
This little recursion trick is so clever. Nice.
} | ||
|
||
if (uri.authority === RELAY_CONFIG_SCHEMA_PATH) { | ||
this.cachedJsonSchema ||= this.loadConfigJsonSchema(); |
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.
If the user changes their relay binary path in the config file, does this get invalidated correctly?
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.
No, but you have to restart the extension anyways because the binary information is only loaded once at startup atm.
|
||
if (binaryVersion) { | ||
const hasConfigJsonSchemaCommand = | ||
semver.satisfies(binaryVersion, '>=18.0') || |
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.
This probably needs to be updated depending on what the release schedule will be like. Just picked the next major for now.
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.
Since it will be in the next release, can we just say >17.0.0
?
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, one more realization. What happens if you use a version of the compiler from the @main NPM tag? Will that work or not? Would be cool to be able to test this end to end before the next release by installing the new version of the extension and installing relay-compiler@main (note that @main releases are currently stuck due to broken CI)
0982908
to
7e70f40
Compare
The Prettier and ESLint rules are conflicting... Nice |
7e70f40
to
8a02bdb
Compare
@@ -25,6 +25,7 @@ module.exports = { | |||
'operator-linebreak': 'off', | |||
'@typescript-eslint/indent': 'off', | |||
'@typescript-eslint/object-curly-spacing': 'off', | |||
'@typescript-eslint/brace-style': 'off', |
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.
Good call.
Verified that it would attempt to load the JSON schema for a prerelease compiler (but it wasn't working of course since there isn't a released version with the |
@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@captbaritone merged this pull request in c434290. |
No description provided.