Skip to content
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

Automatically Generate JSON Schema #4227

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Conversation

tbezman
Copy link
Contributor

@tbezman tbezman commented Feb 21, 2023

The goal of this PR is to begin the journey towards better documentation around Relay configuration.

  1. Automatically generate a JSON Schema which anyone can use in their editor using the schemars crate.
  2. Update the DefinitelyTyped create to export a type for the config so people can use the JSDoc syntax @type import('relay-compiler').ConfigFile syntax to get better completion in relay.config.js files. PR Here

Callouts

  • I refactored some of the serde bits of SingleProjectConfigFile since the struct level default attribute was interfering with the required bit of the JSON Schema. More fine grained default attributes lets us have better autocompletion. Please sanity check me on this work, I want to make sure I didn't break anything
  • fixJsonSchema.js is a script to make sure that we have the proper oneOf field instead of anyOf which schemars is generating without letting us configure it. This may be a footgun but it seems to work well on my editor.

Next Steps

Figure out a sane way to generate markdown documentation + some guided documentation on setting up a relay config. Maybe we could even have a npx relay-compiler init style command.

@tobias-tengler
Copy link
Contributor

Is your goal also to provide the JSON schema via the VS Code extension? I already explored this in #4162, but I got stuck on creating a really great schema that correctly utilizes oneOf to differentiate between multi and single project setups and didn't have much time recently to look into it.
Your approach looks much more integrated. Maybe you can re-use the editor parts I already built :)

@tbezman
Copy link
Contributor Author

tbezman commented Feb 21, 2023

@tobias-tengler My plan was to write some documentation alongside this and recommend people use this in their relay config since most editors support json schemas. I haven't throughly inspected the schema. You down to chat and talk through some of the problems you faced?

@tobias-tengler
Copy link
Contributor

tobias-tengler commented Feb 21, 2023

Sure :) I started by modeling the config options in typescript, so that people using relay.config.js could also profit from the types. Then I tried to derive a JSON schema from that using a npm package, since I only have little experience with Rust. The problem I faced was that the TypeScript to JSON package I used produced anyOf instead of oneOf for unions like the Multi or Single project config. This lead to all options being available in the schema so it was hard to differentiate in the edtior where a property belonged to. Maybe that issue isn't present in the Rust converter you're using...

@tbezman
Copy link
Contributor Author

tbezman commented Feb 21, 2023

@tobias-tengler Wow, great insight! It looks like the package I'm using is having the same issue. I'm looking now to see what kind configuration they support to resolve this. Is it a fundamental issue or should we be able to work around this?

@tobias-tengler
Copy link
Contributor

tobias-tengler commented Feb 21, 2023

I considered doing a find and replace in the schema to just replace all anyOf with oneOf, but it's extremely hacky of course...
I haven't found an option to toggle the behavior in my JS lib, hopefully you have more luck 🤞

@tbezman
Copy link
Contributor Author

tbezman commented Feb 21, 2023

@tobias-tengler Seems like the library we're using makes the decision for us based on some heuristics https://github.com/GREsau/schemars/blob/master/schemars_derive/src/schema_exprs.rs#L274

I manually changed the anyOf to oneOf and VSCode seems to be handling it fine. I don't like this but my god this is so much better than what we have today

@tbezman
Copy link
Contributor Author

tbezman commented Feb 21, 2023

Like, I wouldn't mind writing a short script to change the anyOf to oneOf after generation

@tbezman
Copy link
Contributor Author

tbezman commented Feb 21, 2023

@tobias-tengler Here's an example of what was outputted by this PR https://pastebin.com/nfxAcp5H

@tbezman
Copy link
Contributor Author

tbezman commented Feb 21, 2023

It also looks like we could generate a typescript file using something like https://www.npmjs.com/package/json-schema-to-typescript so people get autocomplete in relay.config.json and relay.config.js

@tobias-tengler
Copy link
Contributor

On my phone so I can't check thoroughly, but at a glance the "schema" (and maybe "src") property should be required in the single config. I think it's not, due to how the config is constructed internally, but as a developer I have to specify them or the compiler panics.

run on push and pull request

try removing on

add name to action

add back on

update the directory

specify musl target

install musl tool

try cargo action

specify manifest path

install libssl

add pkg-config

try another package

vendored feature

remove install packages

generate json schema
@tbezman tbezman changed the title Json schema Automatically Generate JSON Schema Feb 22, 2023
@tbezman tbezman marked this pull request as ready for review February 22, 2023 04:48
Copy link
Contributor

@tobias-tengler tobias-tengler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tbezman Could you upload the JSON schema for me to review as well? I can't checkout and build the code rn.


- name: Generate JSON Schema
run: |
./compiler/target/release/relay-config-schema > website/static/relay-config-schema.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are doing this so people can reference the JSON schema via the Relay website?
In my old PR I opted into including the JSON schema in the relay-compiler npm package, so that the JSON schema would match the installed version of the compiler in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes way more sense and would be versioned appropriately. I'll make this change tonight.

const fileContents = fs.readFileSync(schemaPath, 'utf-8');
const parsedFileContents = JSON.parse(fileContents);

parsedFileContents.oneOf = parsedFileContents.anyOf;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be some nested anyOfs that need to be covered as well if I remember correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call me crazy, but I'm pretty sure all of the anyOfs should be converted to oneOfs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I agree. I don’t think there’s any anyOf in there that can’t be replaced by a oneOf.

facebook-github-bot pushed a commit that referenced this pull request Jun 25, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants