relay icon indicating copy to clipboard operation
relay copied to clipboard

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

Open noghartt opened this issue 1 year ago • 14 comments
trafficstars

resolve https://github.com/facebook/relay/issues/4037

noghartt avatar Jan 07 '24 00:01 noghartt

I love this idea, and I've wanted it for some time. I was hopeful that there would be some way to use serde to derive this, but so far I haven't found such a took that works with our somewhat complicated setup.

I'm a little hesitant to merge this given that I don't think it's fully complete. For example I don't see feature flags or mutli-project config. Would you be interested in taking it the rest of the way and capturing the full config? If so, I think this would be a great addition.

If we do go this route, I'd love to add a few more things:

  1. Comments on the relevent config structs reminding people that any changes to the struct should also be added to these types.
  2. Documentation updates adding the types as comments, similar to what next-js does (/** @type {import('next').NextConfig} */

Thanks for taking the time to propose this!

captbaritone avatar Jan 09 '24 17:01 captbaritone

I'm a little hesitant to merge this given that I don't think it's fully complete. For example I don't see feature flags or mutli-project config. Would you be interested in taking it the rest of the way and capturing the full config? If so, I think this would be a great addition.

I'd love to take the rest of the config and documentate it.

But yes, it isn't fully complete because I didn't find some good documentations related to the rest of the config. I saw that the relay-config crate has other fields, but I couldn't find anything that documentate these configurations, any idea where I can find it?

What I follow was the README from the react-compiler package, but I think that is a little outdated, right?

noghartt avatar Jan 09 '24 18:01 noghartt

If it's just about adding the TypeScript declarations, I think you can work of the declaration file I derived from the config last year: https://github.com/facebook/relay/blob/d873d5b8927b4a5bb6ca2c4b9a4c504ea0723b6b/packages/relay-compiler/index.d.ts I would also like to continue my effort regarding the VSCode integration for config completion in JSON files: https://github.com/facebook/relay/pull/4162 Now that I have some more experience with the compiler, maybe I have more luck than back then :D

tobias-tengler avatar Jan 09 '24 18:01 tobias-tengler

@noghartt

I couldn't find anything that documentate these configurations, any idea where I can find it?

Sadly the best source of truth is the Rust crate. Luckily most of the fields are commented with public-facing comments. You can find it here: https://github.com/facebook/relay/blob/main/compiler/crates/relay-compiler/src/config.rs#L931

You'll need to learn a bit about how Serde (which is a Rust crate that can derived a parser for a Rust structure based on its definition) works to understand what the JS/JSON shape will look like. The main thinks to look out for:

  1. Each child value/object will have its own deserialization logic, usually derived from a Serde derived trait at the structs declaration (simple values like strings/paths/etc generally have intuitive inputs)
  2. rename_all = "camelCase" converts snake_case names to camelCase
  3. #[serde(flatten)] will push all the keys of the child object up into the parent object.

Thank you so much for your interest in this. The lack of documentation for the Relay config is a huge gap, and TypeScript types will offer both in-editor support and human readable documentation that we can point people to.

captbaritone avatar Jan 09 '24 18:01 captbaritone

cc @captbaritone @tobias-tengler

I improved the types and wrote more JSDocs based on the config.rs as mentioned before. Improve the doc too, what do you think?

noghartt avatar Jan 10 '24 17:01 noghartt

Another point: We don't have authoritative docs on this config. As a baseline we should link out to this definition from our docs as a "Here's where you can go to find out what options are available in the config". As a stretch, I wonder if it would be possible to use something like https://www.npmjs.com/package/docusaurus-plugin-typedoc to generate an actual docs page (or pages) from this?

Maybe we can start with the former and then (optionally) explore the later in a separate PR.

captbaritone avatar Jan 10 '24 18:01 captbaritone

Another point: We don't have authoritative docs on this config. As a baseline we should link out to this definition from our docs as a "Here's where you can go to find out what options are available in the config". As a stretch, I wonder if it would be possible to use something like https://www.npmjs.com/package/docusaurus-plugin-typedoc to generate an actual docs page (or pages) from this?

Maybe we can start with the former and then (optionally) explore the later in a separate PR.

Would be really cool if we could have a docs specifically for these configs, explaining which things each field are doing. I think that we can derive some docs from this config, like one about feature flags and what are the features related.

noghartt avatar Jan 10 '24 21:01 noghartt

Anything else I need to fix, what do you think?

cc @captbaritone @tobias-tengler

noghartt avatar Jan 15 '24 13:01 noghartt

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Jan 18 '24 00:01 facebook-github-bot

I've imported this, but it will race with https://github.com/facebook/relay/pull/4585 which changes the config types. I'll try to fix it up internally before I land.

captbaritone avatar Jan 18 '24 00:01 captbaritone

Hey. After importing this, I'm seeing a bunch more errors. How have you been validating this addition? Could you find a way to ensure it's working end to end on a given config file? Maybe include a screenshot of what errors look like in VSCode?

captbaritone avatar Jan 18 '24 00:01 captbaritone

@noghartt Can you please add a test plan that demonstrates this working end to end? For example, a screenshot of it reporting errors on an invalid config file in VSCode? (and one of it not erroring on a valid config file)?

captbaritone avatar Jan 18 '24 00:01 captbaritone

@noghartt Can you please add a test plan that demonstrates this working end to end? For example, a screenshot of it reporting errors on an invalid config file in VSCode? (and one of it not erroring on a valid config file)?

Of course. But in case, VSCode and LSP does not infer type checking over type infered objects that comes from JSDocs annotation, so it'll won't trigger a visual error for the final user, only if they have a relay.config.ts and let the tsserver does it for us.

But in case, I took some screenshots that shows this type working, I'll let also a test plan containing how I test it:

Test plan

  • [ ] build the packages using gulp with the command yarn gulp mainrelease
  • [ ] create a new relay.config.js file
  • [ ] add type annotations targeting the exported RelayConfig type from ./dist build package

the boilerplate

// relay.config.js

/** @type {import('./dist/relay-compiler').RelayConfig} */
module.exports = {
  // ...
}

Screenshots

starting single project image

multi project config image

feature flags image


As an example, even the next.config.js aren't being typechecked by the LSP.

image

noghartt avatar Jan 18 '24 01:01 noghartt

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Jan 18 '24 17:01 facebook-github-bot

Well done by @captbaritone on this PR https://github.com/facebook/relay/pull/4723. We can follow this improvement and bring some type emission following the ideas of JsonSchema!

noghartt avatar Jun 25 '24 23:06 noghartt