Add support for Typescript in Rust Relay compiler #3182

MaartenStaa · 2020-09-11T08:58:49Z

This PR adds a language option to the typegen_config and refactors the relay-typegen crate to allow completely different output based on the configured language, currently typescript and flow (which is the default when no language is configured, or an invalid one is set).

I also made some changes so the watchman query looks for .ts files when using typescript, and the output files also use .ts as their extension.

This probably needs a bit more work before it's completely ready. Off the top of my head:

In relay-compiler/build_project/artifact_content.rs some output is generated such as @flow and a commented out import type statement? Those should probably be left out for Typescript.
Currently the language used for typegen is Typescript if the language in the config file is "typescript", and Flow otherwise. We should probably display an error if an invalid language is set, e.g. "typesrcipt".
I was not able to actually test the compiler due to an error returned by Watchman: failed to parse query: must use ["suffix", "suffixstring"], and I don't have a clue why. This error was already returned before I made the changes to look for .ts files when using Typescript.
I did not at all look into relay-lsp and whether any changes are needed there for Typescript.

Looking forward to your feedback!

cc @kassens @josephsavona @maraisr

… different language can be inserted.

…age.

josephsavona · 2020-09-11T15:06:49Z

Awesome! I took a quick look through and the high-level approach makes sense, of having typegen create an abstract AST of types and then having Flow/TS-specific writers to convert that AST to code. Thanks for the contribution!

I'll leave to @tyao1, @kassens, and @rbalicki2 for review

zth · 2020-09-12T09:45:27Z

This is so cool, awesome work @MaartenStaa ! 😍

facebook-github-bot

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

maraisr · 2020-09-13T07:05:17Z

compiler/crates/relay-compiler/src/watchman/query_builder.rs

+                    // Ending in *.js or *.ts depending on the project lanague.
+                    Expr::Suffix(vec![match &p.typegen_config.language {
+                        TypegenLanguage::Flow => "js",
+                        TypegenLanguage::Typescript => "ts",


with react, this has to be tsx. So could this be an array perhaps?

You're right, I adjusted it! I set it to look at .jsx as well, just in case people use that.

Edit: I committed this on my phone and see that it doesn't compile. I'll have to take a look at that later. I'm on vacation now so a bit limited.

@maraisr Fixed the build now as well.

kassens

First, I want to repeat everyone here and say this is awesome and pretty much aligns with how I thought it'd be done ideally.

Did you come up with the way the fragment references are modeled in typescript or was this based on the existing (community driven) typescript types? I know this was pretty tricky to get right in Flow.

kassens · 2020-09-14T13:27:08Z

compiler/crates/relay-typegen/src/config.rs

+
+impl Default for TypegenLanguage {
+    fn default() -> Self {
+        Self::Flow


I was even thinking of making Typescript the default, but just realized *.ts files wouldn't work with vanilla JS anymore. The Flow generated *.js files using Flow comment style syntax allows people to just treat them as pure JS files.

Maybe we could even make this optional and skip generating types altogether to make the files a bit smaller.

kassens · 2020-09-14T13:32:06Z

compiler/crates/relay-compiler/src/watchman/query_builder.rs

+                    Expr::Suffix(match &p.typegen_config.language {
+                        TypegenLanguage::Flow => vec![PathBuf::from("js"), PathBuf::from("jsx")],
+                        TypegenLanguage::Typescript => {
+                            vec![PathBuf::from("ts"), PathBuf::from("tsx")]


For a TS project, I suppose you'd have all your own product code in *.tsx? files or should this also include .js files for hybrid projects?

I think generally its all ts or all js. But can't presume that, so think yeah maybe hybrids are nice?

facebook-github-bot

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

alloy · 2020-09-14T15:03:22Z

Awesome, very nice to see this @MaartenStaa 👏

alloy · 2020-09-14T15:08:18Z

...r/crates/relay-typegen/tests/generate_typescript/fixtures/unmasked-fragment-spreads.expected

+  readonly $fragmentRefs: RecursiveFragment$ref,
+};
+-------------------------------------------------------------------------------
+import type { PhotoFragment$ref } from "PhotoFragment.graphql";


It would be better to avoid any need to import these types, as that entirely removes the need to have a single artefact directory (or Haste). See for example this output of the existing TS plugin.

Yeah... I think this is actually a blocker for a merge. Typescript wouldn't know what that means (please correct me here).

At the very least just prefix with a ./,

-import type { PhotoFragment$ref } from "PhotoFragment.graphql"; +import type { PhotoFragment$ref } from "./PhotoFragment.graphql";

Hmm, yeah that’s a good point. @kassens can you amend that in your import?

kassens · 2020-09-15T18:14:53Z

I'd like to just have TS output be included as I'm not sure of a good way to make it pluggable and maintainable.
Unfortunately, I (and the Rest of FB Relay engineers) unfortunately have limited experience and time to polish the TS output and figure out how it's supposed to work.
This PR seems like a great start and make it easier to folks to start contributing changes.

Given the above, I'm somewhat inclined to merge this and have it open up for contributions to tweak the output in follow up PRs. Thoughts?

facebook-github-bot

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

maraisr · 2020-09-17T00:37:29Z

Yeah @kassens I think that is a great way forward. Also a great way for non-Rust contributors to have a go, as is not as daunting as starting something from scratch. (Mind you @MaartenStaa did just that 😛).

I'm super keen to investigate a cleaner named interface, and proper union types. And think it was outlined in another ticket already, around not merging unions and forcing things to be nullable.

maraisr · 2020-09-17T00:47:53Z

compiler/crates/relay-typegen/tests/generate_typescript/fixtures/conditional.expected

+  readonly $fragmentRefs: ConditionField$ref,
+};
+-------------------------------------------------------------------------------
+import { FragmentReference } from "relay-runtime";


think though maybe this should import type {} no?

Not a requirement, but definitely something to follow up with.

@maraisr correct. It was a conscious decision to use this option since import type was a relatively recent addition to Typescript, and I wouldn't want to make this unusable for people on older versions.

The fragment reference import does currently use the type keyword, though.

MaartenStaa · 2020-09-17T07:56:04Z

Given the above, I'm somewhat inclined to merge this and have it open up for contributions to tweak the output in follow up PRs. Thoughts?

This sounds like a reasonable way to move forward, though I feel somewhat uneasy about having this merged at this stage since I wasn't able to test this yet.

(Mind you @MaartenStaa did just that 😛).

To be fair, I have played around with Rust before in my spare time, so it's not like I dove in with zero experience.

I'm super keen to investigate a cleaner named interface, and proper union types. And think it was outlined in another ticket already, around not merging unions and forcing things to be nullable.

I think you may be referring to my other PR, and how we ended up here in the first place: #3178.

alloy · 2020-09-17T09:37:41Z

This sounds like a reasonable way to move forward, though I feel somewhat uneasy about having this merged at this stage since I wasn't able to test this yet.

I think it would be ok to merge (with at least the relative import change) and then it would be good to port e.g. the TS example that exists in the TS plugin repo to use the rust compiler and then iterate on any issues and the few discussed here over the next few weeks. Others (e.g. me) can chime in too.

As long as the relay team is ok with not putting out a public release until the dust has settled (@kassens ?).

@MaartenStaa What do you think?

facebook-github-bot · 2020-09-18T00:36:01Z

@kassens merged this pull request in ab5c96b.

mrjackdavis · 2022-01-09T08:30:58Z

For those of you coming here from google to fix this error:

Watchman error: Watchman error: The watchman server reported an error: "failed to parse query: must use ["suffix", "suffixstring"]", while executing command

I had to upgrade watchman

brew upgrade watchman

And restart watchman

watchman shutdown-server

Maarten Staa added 10 commits September 10, 2020 10:42

Extract AST and Props from flow to separate file.

1dd9346

Remove direct dependency to Flow print_type, use a trait instead so a…

bc7292e

… different language can be inserted.

Add typescript printer.

6c2fcd6

Better support for nullable types/optional fields, and read only arrays.

5840f10

Typescript makes no difference between exact and inexact types.

3cf9cab

Introduce new ASTs in order to not adapt all outputted code per langu…

c7ffa7d

…age.

Add typescript version of typegen fixtures.

fbce1e7

Take project language into account when building watchman query.

1fa6191

Use better function name for writer trait in relay-typegen.

5ed9225

Take project language into account for artifact extension.

d40a34a

facebook-github-bot added the CLA Signed label Sep 11, 2020

Use enum for TypegenLanguage for built-in validation from Serde.

96e771e

facebook-github-bot reviewed Sep 13, 2020

View reviewed changes

maraisr reviewed Sep 13, 2020

View reviewed changes

MaartenStaa and others added 2 commits September 13, 2020 10:08

Look at jsx and tsx extensions as well

54dd5bf

Fix compile error.

2520ae5

kassens reviewed Sep 14, 2020

View reviewed changes

facebook-github-bot reviewed Sep 14, 2020

View reviewed changes

alloy reviewed Sep 14, 2020

View reviewed changes

facebook-github-bot reviewed Sep 16, 2020

View reviewed changes

maraisr reviewed Sep 17, 2020

View reviewed changes

facebook-github-bot closed this in ab5c96b Sep 17, 2020

facebook-github-bot added the Merged label Sep 18, 2020

kassens mentioned this pull request Sep 23, 2020

[epic] enable Rust compiler for OSS #3180

Closed

13 tasks

This was referenced Jun 13, 2021

chore(deps): update dependency relay-compiler to v10.1.3 - autoclosed relay-tools/relay-graphql-js#16

Closed

chore(deps): update dependency relay-config to v10.1.3 - autoclosed relay-tools/relay-graphql-js#17

Closed

saihaj mentioned this pull request Dec 16, 2021

Graphql v16 (or higher) does not work with Relay graphql/graphql-js#3429

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Typescript in Rust Relay compiler #3182

Add support for Typescript in Rust Relay compiler #3182

MaartenStaa commented Sep 11, 2020 •

edited

Loading

josephsavona commented Sep 11, 2020

zth commented Sep 12, 2020

facebook-github-bot left a comment

maraisr Sep 13, 2020

MaartenStaa Sep 13, 2020 •

edited

Loading

MaartenStaa Sep 13, 2020

kassens left a comment

kassens Sep 14, 2020

kassens Sep 14, 2020

maraisr Sep 17, 2020

facebook-github-bot left a comment

alloy commented Sep 14, 2020

alloy Sep 14, 2020

maraisr Sep 17, 2020

alloy Sep 17, 2020

kassens commented Sep 15, 2020

facebook-github-bot left a comment

maraisr commented Sep 17, 2020

maraisr Sep 17, 2020

alloy Sep 17, 2020

MaartenStaa Sep 17, 2020

alloy Sep 17, 2020

MaartenStaa commented Sep 17, 2020

alloy commented Sep 17, 2020 •

edited

Loading

facebook-github-bot commented Sep 18, 2020

mrjackdavis commented Jan 9, 2022

Add support for Typescript in Rust Relay compiler #3182

Add support for Typescript in Rust Relay compiler #3182

Conversation

MaartenStaa commented Sep 11, 2020 • edited Loading

josephsavona commented Sep 11, 2020

zth commented Sep 12, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaartenStaa Sep 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kassens left a 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

facebook-github-bot left a comment

Choose a reason for hiding this comment

alloy commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kassens commented Sep 15, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

maraisr commented Sep 17, 2020

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

MaartenStaa commented Sep 17, 2020

alloy commented Sep 17, 2020 • edited Loading

facebook-github-bot commented Sep 18, 2020

mrjackdavis commented Jan 9, 2022

MaartenStaa commented Sep 11, 2020 •

edited

Loading

MaartenStaa Sep 13, 2020 •

edited

Loading

alloy commented Sep 17, 2020 •

edited

Loading