-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(compiler): add type declaration with relay config file types #4569
base: main
Are you sure you want to change the base?
Conversation
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:
Thanks for taking the time to propose this! |
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 What I follow was the README from the |
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 |
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.
cool
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:
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. |
cc @captbaritone @tobias-tengler I improved the types and wrote more JSDocs based on the |
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.
So exciting! This is going to be such a good improvement. A few requests:
- Could you do another pass to ensure no properties which are actually optional are marked as non-optional?
- In the Rust code, can you add a comment at the top of each struct that is part of this config with a note that any changes made to the struct should also be made to these types?
packages/relay-compiler/index.d.ts
Outdated
variableNamesComment?: boolean; | ||
featureFlags?: RelayConfigFeatureFlags; | ||
/** A placeholder for allowing extra information in the config file */ | ||
extra: unknown; |
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.
Presumably this should be optional?
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 should probably just be [configKey: string]: any
?
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.
I pushed a fix into this tht turns it into an optional field.
This should probably just be
[configKey: string]: any
?
In case, I think that if we should type it, we should type accordingly to the serde_json::Value
struct, that can be any JSON-friendly value. What do you think?
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. |
Anything else I need to fix, what do you think? |
packages/relay-compiler/index.d.ts
Outdated
*/ | ||
jsModuleFormat?: 'commonjs' | 'haste'; | ||
/** Options for configuring the output of compiler diagnostics. */ | ||
diagnosticReportConfig: RelayConfigDiagnosticReport; |
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.
I think we need to understand defaults a little better, since I know a config without this value (for example, but others as well) is accepted by the compiler.
Ah, looking a bit more closely, it looks like the struct itself is annotated with default
. Can you check each of the structs and see how that impacts these types?
#[serde(deny_unknown_fields, rename_all = "camelCase", default)] |
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.
I checked all the structs and update some JSDocs with new @default
values. I think that everything is OK now, what do you think?
@@ -74,7 +75,7 @@ module.exports = { | |||
``` | |||
|
|||
This configuration also can be specified in `"relay"` section of the `package.json` file. | |||
For more details, and configuration options see: [Relay Compiler Configuration](https://github.com/facebook/relay/tree/main/packages/relay-compiler) | |||
For more details, and configuration options see: [Relay Compiler Configuration](https://github.com/facebook/relay/tree/main/packages/relay-compiler/index.d.ts) |
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.
Love this! Could you also add a similar note to https://github.com/facebook/relay/blob/main/packages/relay-compiler/README.md#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.
Added! I updated the README
with a note admonition containing the same info.
@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
I've imported this, but it will race with #4585 which changes the config types. I'll try to fix it up internally before I land. |
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? |
packages/relay-compiler/index.d.ts
Outdated
| { | ||
/** The path of your schema file. */ | ||
schema: string | ||
}; |
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.
I don't think a semicolon is valid here.
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.
I think that this isn't a problem, but I removed it to follow the pattern
packages/relay-compiler/index.d.ts
Outdated
* ``` | ||
*/ | ||
diagnosticReportConfig?: RelayConfigDiagnosticReportConfig; | ||
featureFlags?: RelayFeatureFlags; |
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 type is not defined.
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.
Fixed
packages/relay-compiler/index.d.ts
Outdated
* (TypeScript only) Whether to use the `import type` syntax introduced in Typescript version 3.8. This will prevent warnings from `importsNotUsedAsValues`. | ||
*/ | ||
useImportTypeSyntax?: boolean; | ||
customScalarTypes?: CustomScalarTypes; |
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 type is not defined
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.
Fixed
packages/relay-compiler/index.d.ts
Outdated
* | ||
* @default false | ||
*/ | ||
noFutureProofEnums?: NoFutureProofEnums; |
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 type is not defined
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.
I defined this type, fixed
packages/relay-compiler/index.d.ts
Outdated
* inlined or not depends on whether it actually uses that directive. | ||
*/ | ||
no_inline?: RelayConfigFeatureFlag; | ||
enable_3d_branch_arg_generation?: bool; |
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.
bool
is not a valid TypeScript type.
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.
Oh, nice catch, fixed
packages/relay-compiler/index.d.ts
Outdated
/** Create normalization nodes for client edges to client objects. */ | ||
emit_normalization_nodes_for_client_edges?: boolean; | ||
/** Fully build the normalization AST for Resolvers. */ | ||
enable_resolver_normalization_ast?: bool; |
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.
bool
is not a valid TypeScript type.
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.
Fixed
@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 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
the boilerplate // relay.config.js
/** @type {import('./dist/relay-compiler').RelayConfig} */
module.exports = {
// ...
} ScreenshotsAs an example, even the |
@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@@ -17,6 +17,7 @@ use serde::Serialize; | |||
|
|||
use crate::Rollout; | |||
|
|||
/// **NOTE**: Every change on this struct should be reflected into the `relay-compiler/index.d.ts` file. |
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.
I wonder if we can make use of something like this: https://github.com/Aleph-Alpha/ts-rs here?
We would generate these typescript definitions based on the actual rust types?
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.
I did some POCs with specta yesterday, it seems to works well. I think that we can bring it to these structs too.
They have just a limitation around some JSDocs comments, like the @default
and others, but it's working.
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.
Nice, @noghartt! I think this would be a much better solution to always keep these definitions in sync.
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.
@noghartt Just checking in here. Were you able to make any progress with Spectra? Do you think it can offer a viable automated solution?
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.
@noghartt Just checking in here. Were you able to make any progress with Spectra? Do you think it can offer a viable automated solution?
I think that we can bring an automated solution yes. But as I commented, specta doesn't seem to fit all the types for us, as an example, it doesn't fill the JSDocs related fields, like the @default
. I couldn't find a workaround for that scenario.
Except that, it seems to works well. Do you think that we can bring this improvement in this PR or maybe moving to another?
resolve #4037