[VSCode] Provide autocompletion for compiler configuration #4162

tobias-tengler · 2022-12-30T22:25:53Z

When the VSCode extension is active, it will now autocomplete configuration options of the compiler, both in the relay.config.json and the package.json.

Single project

Multi project

tobias-tengler

I created the JSON schema by hand, since I thought the configuration doesn't change that often. Investing in automation felt like a waste of time...

tobias-tengler · 2022-12-30T22:46:50Z

vscode-extension/src/json-schemas/relay-config-json.jsonc

+    "FeatureFlags": {
+      "type": "object",
+      "properties": {
+        "enable_flight_transform": {
+          "type": "boolean"
+        },
+        "enable_relay_resolver_transform": {
+          "type": "boolean"
+        },
+        "use_named_imports_for_relay_resolvers": {
+          "type": "boolean",
+          "default": true
+        },
+        "relay_resolver_model_syntax_enabled": {
+          "type": "boolean"
+        },


Should feature flags be properly typed or should I just not type them as they are likely very transient?

captbaritone · 2023-01-03T17:45:05Z

This is so rad! I'm a little nervous about this falling out of sync with our config, but given that our config is already poorly documented, having this to help people out seems very worthwhile. One thing to look into: We use serde to define our config serialization/deserialization. I wonder if there are any existing tools to auto generate json schema from serde definitions?

If that does not exist, I wonder if there's some way to test that this definition stays in sync with our existing config schema?

tobias-tengler · 2023-01-03T17:57:14Z

@captbaritone I thought about that and researched some tools, but all of them would require some manual modification of the schema after it was generated. The Rust config is also interleaved with options that aren't publicly exposed and just used for internal state.
I assumed that the existing options weren't likely to be changed or removed (often) and it wouldn't be that bad to maybe miss a new minor option. If they do get out of sync, I think we could think about automating it. But as it's just an assisting tool and it getting out of sync doesn't break anything, I think it's not worth the effort at the moment. I think it should suffice if someone knows about the existence of these files and can raise a comment during code review if they spot them not being updated alongside a configuration change....

One thing I was thinking about was that it would be nice to ship the JSON schema alongside the compiler, so that the autocompletion always reflects the set of options the compiler exposes and not just the latest version of those options. I'll play around with it a bit.

captbaritone · 2023-01-03T18:13:42Z

Given that the LSP is:

Already watching files and capable of providing diagnostics
Knows which file is actually the config file (not just based on filename matching)
Knows how to parse and report errors on the config format

It does seem like we could find a way to have our main LSP process report errors. In fact, I believe we try to parse the config on startup and currently just crash. Maybe it would be sufficient to find that code path and look into converting the serde parse error into a DiagnositcError.

I think that happens here:

relay/compiler/crates/relay-bin/src/main.rs

Line 312 in 2ac8438

let config = get_config(command.config)?;

Maybe start_language_server could accept some kind of Result of the config, or even just the config path and then do custom error handling?

Related, ideally the LSP and compiler in watch mode could actively watch the config file for changes and either automatically restart, or at least prompt for a manual restart.

tobias-tengler · 2023-01-03T18:39:28Z

@captbaritone I think this would be great, if the compiler can actually surface wrong configuration directly in the file. I still think the JSON schema has its place though. I mainly introduced it for discovery purposes and assistance in writing that configuration. I think the LSP can take over after that and help me out when there are logical errors in my configuration, like invalid paths. But honestly I think the error reporting is plenty sufficient already, it's mostly about discovery and ease of use.

captbaritone · 2023-01-03T18:51:53Z

Good point. This provides autocomplete in addition to error reporting.

captbaritone · 2023-01-04T19:00:35Z

One thing I was thinking about was that it would be nice to ship the JSON schema alongside the compiler, so that the autocompletion always reflects the set of options the compiler exposes and not just the latest version of those options. I'll play around with it a bit.

Sounds good. Let me know when you're ready for this PR to get looked at again.

tobias-tengler

@captbaritone Okay so what I've done now is remove the JSON schema from the VS Code extension and placed it in the relay-config crate. Being colocated with the actual config should hopefully help with reminding other developers to also update the schema.
This schema file is then copied via gulp to the relay-compiler NPM package. The VS Code extension already has a mechanism to locate the compiler binary in the node_modules, so I'm just piggybacking off of that to also locate the config-schema.json. If it exists it will be used to provide auto-completion in Relay's JSON based configuration files.
With this the config auto-completion should always match the compiler version.

gulpfile.js

vscode-extension/src/providers/textDocumentContentProvider.ts

captbaritone · 2023-01-09T17:23:56Z

compiler/crates/relay-config/src/config-schema.json

@@ -0,0 +1,588 @@
+{
+  "$schema": "http://json-schema.org/draft-07/schema",


Do you think we could add a comment above each struct/enum that is included here informing folks that changes made to that struct need to be replicated here?

This is also making me think that for folks who describe their config using typescript, it would be nice to have typescript definitions as well. Are there tools to generate typescript from json-schema (or vise-versa?)

I've used a JSON Schema to TypeScript npm package before, but I'm sure it exists the other way around as well.
So you're saying there should be TypeScript types for the config.
Where should these live? @types/relay-compiler? I think an entirely different repository will be even harder to sync. I'm personally of the opinion that all TS types should be inlined into the Relay repository, so I could also just add it straight to the relay-compiler package. We could then have a script that created the JSON schema from these TypeScript definitions at build, so that only the TS types have to be kept in sync.

Yeah, I'd be onboard with them living in the relay-compiler package directly.

Okay I've added the TypeScript types and tested two packages to generate the JSON schema from the TypeScript types. In both cases the schema is losing expressiveness in the form oneOf. With the handwritten schema you could for example express that only "schema" or "schemaDir" is supported, which is now no longer possible. If we go the reverse and generate the Types from the JSON schema, the types are also weird in some places.
Is this loss of expressiveness okay in light of the easier maintainability? I guess it's still better to have some guidance, but it could also be confusing because both Single project and Multiproject options are available at the root, which would lead to the compiler panicking...

What's the reason for needing two scripts to generate the schema? I don't follow that bit.

I suspect the best fix for the oneOf issue would be to move to a config format that was less overloaded.

What do you personally think of the tradeoff of manual vs generated? It seems like the generated one is still much better than nothing, even if it's not optimal, and that it's better than having to maintain two different versions, but I haven't tried it out enough to know how badly the generated schema misses the mark.

It's just to showcase what two different packages would output. This is not final. The generated JSON file would also not be checked in normally and the build integration is still missing. I'll mark as draft again to avoid confusion.

I would prefer the generated, if it was a bit better.
The goal of this was to assist you in writing the config file. It does that, but the oneOf issue suggested invalid configuration options that might lead to errors, which is bad. I'll try to see if there is something I can do to improve the situation, but I don't have anymore time today, sadly...

gulpfile.js

captbaritone · 2023-01-09T19:55:26Z

vscode-extension/src/providers/register.ts

+
+  const isPrerelease = semver.prerelease(packageManifest.version) != null;
+
+  if (!isSemverRangeSatisfied && !isPrerelease) {


Seems like it would make more sense to do this early exit before we try to read the file.

I'm not sure I'm following... We have to read the packageJson file above to find the version to do the semver checks. If the version doesn't match we don't register the provider and it will never attempt to read the JSON schema.

captbaritone · 2023-01-09T20:10:16Z

compiler/crates/relay-config/src/config-schema.json

@@ -0,0 +1,588 @@
+{
+  "$schema": "http://json-schema.org/draft-07/schema",


What's the reason for needing two scripts to generate the schema? I don't follow that bit.

I suspect the best fix for the oneOf issue would be to move to a config format that was less overloaded.

What do you personally think of the tradeoff of manual vs generated? It seems like the generated one is still much better than nothing, even if it's not optimal, and that it's better than having to maintain two different versions, but I haven't tried it out enough to know how badly the generated schema misses the mark.

Summary: One huge usability issue with Relay today is that the compiler config is wholely undocumented. To understand what options are available you either need to _try_ an option and try to decipher the sometime cryptic validation errors, or read the Rust code. This PR attempts to start to close that gap. Our Rust structs and comments _are_ a good source of truth for this info, so ideally we still use that as our documentation/validation. So, here we derive [Json Schema](https://json-schema.org/) from our Rust structs/enums/types/serde tags. In VSCode this allows for nice autocomplete, hover tool tips, and yellow squiggles if you make an error (much nicer than a cryptic validation error when you run the compiler). Some next steps this could enable: 1. Enable this editor experience directly if you have the Relay VSCode extension installed via the [JSON Schema contribution point](https://code.visualstudio.com/api/references/contribution-points#contributes.jsonValidation) 2. Derive a docs page about the [Docusaurus JSON Schema plugin](https://jy95.github.io/docusaurus-json-schema-plugin/) https://github.com/facebook/relay/assets/162735/7f5e807b-069c-4344-9e9c-506f3369b042 ## Next Steps There are a few unanswered things here that we'll need to consider: 1. How do we publish this schema such that users can find it and ensure that they use the correct schema for the version of the compiler they are using? 1. Especially tricky if we want to enable it automatically in VSCode 2. Maybe we could have the compiler validate that you've added the right value for `$schema`? 3. Can we get the docs plugin to work? (I think it might require Docusaurus 3) 4. Are there bugs? There are some serde attributes and little deserialization tricks which we might not have modeled correctly here. Difficult to know if we've captured them all. 5. Add more descriptions to our structs. Some fields are undocumented. This would provide us more incentive to get those struct/field/variant `///` comments very helpful and up to date. ## Acknowledgment This PR aims to be a more maintainable and reliable alternative to #4569. Thanks for noghartt for pushing the discussion of how we might fill this gap. This approach was also previously explored in other PRs: * #4227 by tbezman * #4162 by tobias-tengler Pull Request resolved: #4723 Reviewed By: tyao1 Differential Revision: D58936067 Pulled By: captbaritone fbshipit-source-id: c58488da4981eed285ee705ac65321146cfb576b

tobias-tengler added 2 commits December 30, 2022 20:41

Single project config autocompletion

9333f33

Multi project autocompletion

89b56da

facebook-github-bot added the CLA Signed label Dec 30, 2022

tobias-tengler marked this pull request as ready for review December 30, 2022 22:44

Resolve todos

97caefb

tobias-tengler commented Dec 30, 2022

View reviewed changes

Merge branch 'main' into tte/config-auto-completion

0dccf52

tobias-tengler marked this pull request as draft January 3, 2023 18:08

tobias-tengler added 3 commits January 6, 2023 11:19

Merge branch 'main' into tte/config-auto-completion

17373c0

Skip search of relay binary, when pathToRelay is specified

f74da46

Load JSON schema from relay-compiler package

87bbf4e

tobias-tengler marked this pull request as ready for review January 6, 2023 12:02

tobias-tengler commented Jan 6, 2023

View reviewed changes

gulpfile.js Outdated Show resolved Hide resolved

tobias-tengler added 2 commits January 6, 2023 13:21

Properly configure prettier eslint rules

d27e730

Merge branch 'main' into tte/config-auto-completion

1d12ab2

captbaritone reviewed Jan 9, 2023

View reviewed changes

tobias-tengler added 2 commits January 9, 2023 20:24

Add TypeScript types

7af32db

Add feature detection

26f0ba2

tobias-tengler force-pushed the tte/config-auto-completion branch from 7c93e85 to 26f0ba2 Compare January 9, 2023 19:51

Merge branch 'main' into tte/config-auto-completion

19ac18c

captbaritone reviewed Jan 9, 2023

View reviewed changes

tobias-tengler marked this pull request as draft January 9, 2023 20:46

Tweak schema generation

d873d5b

tobias-tengler mentioned this pull request Feb 21, 2023

Automatically Generate JSON Schema #4227

Open

tobias-tengler mentioned this pull request Jan 9, 2024

feat(compiler): add type declaration with relay config file types #4569

Closed

tobias-tengler closed this Jun 23, 2024

captbaritone mentioned this pull request Jun 23, 2024

Generate JSON schema for compiler config #4723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VSCode] Provide autocompletion for compiler configuration #4162

[VSCode] Provide autocompletion for compiler configuration #4162

tobias-tengler commented Dec 30, 2022

tobias-tengler left a comment

tobias-tengler Dec 30, 2022

captbaritone commented Jan 3, 2023

tobias-tengler commented Jan 3, 2023 •

edited

Loading

captbaritone commented Jan 3, 2023

tobias-tengler commented Jan 3, 2023

captbaritone commented Jan 3, 2023

captbaritone commented Jan 4, 2023

tobias-tengler left a comment •

edited

Loading

captbaritone Jan 9, 2023

captbaritone Jan 9, 2023

tobias-tengler Jan 9, 2023 •

edited

Loading

captbaritone Jan 9, 2023

tobias-tengler Jan 9, 2023

captbaritone Jan 9, 2023

tobias-tengler Jan 9, 2023

captbaritone Jan 9, 2023

tobias-tengler Jan 9, 2023

captbaritone Jan 9, 2023

		@@ -0,0 +1,588 @@
		{
		"$schema": "http://json-schema.org/draft-07/schema",


		const isPrerelease = semver.prerelease(packageManifest.version) != null;

		if (!isSemverRangeSatisfied && !isPrerelease) {

[VSCode] Provide autocompletion for compiler configuration #4162

[VSCode] Provide autocompletion for compiler configuration #4162

Conversation

tobias-tengler commented Dec 30, 2022

tobias-tengler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captbaritone commented Jan 3, 2023

tobias-tengler commented Jan 3, 2023 • edited Loading

captbaritone commented Jan 3, 2023

tobias-tengler commented Jan 3, 2023

captbaritone commented Jan 3, 2023

captbaritone commented Jan 4, 2023

tobias-tengler left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobias-tengler Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobias-tengler commented Jan 3, 2023 •

edited

Loading

tobias-tengler left a comment •

edited

Loading

tobias-tengler Jan 9, 2023 •

edited

Loading