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

[epic] enable Rust compiler for OSS #3180

Closed
11 of 13 tasks
kassens opened this issue Sep 10, 2020 · 16 comments
Closed
11 of 13 tasks

[epic] enable Rust compiler for OSS #3180

kassens opened this issue Sep 10, 2020 · 16 comments
Labels
pinned Pinned issues will never be marked stale by stalebot

Comments

@kassens
Copy link
Member

kassens commented Sep 10, 2020

This issue collects what I think is missing to update the Relay compiler to the Rust version in OSS. If someone want's to give any of this a shot, please comment here!

facebook-github-bot pushed a commit that referenced this issue Sep 17, 2020
Summary:
See #3181 and #3180.

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.
- [x] 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

Pull Request resolved: #3182

Reviewed By: tyao1

Differential Revision: D23670620

Pulled By: kassens

fbshipit-source-id: 0e3e9e6860c6701d934b406c895dc04bfb5b6322
@kassens
Copy link
Member Author

kassens commented Sep 23, 2020

@MaartenStaa made a huge leap start on this with #3182!

I understand there's a few improvements we can make to the types still by copying more what the JS OSS typescript generator produces: https://github.com/relay-tools/relay-compiler-language-typescript/blob/e1b092f081d4e816ecd17fb0d3b4e62cdf3db917/test/__snapshots__/TypeScriptGenerator-test.ts.snap

I'll file separate issues for the remaining checkboxes.

@kassens
Copy link
Member Author

kassens commented Sep 30, 2020

A good intro way to contribute is #3199 to convert the runtime unit tests to use artifacts generated by the Rust compiler. More details in there.

@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Dec 25, 2020
@alunyov alunyov removed the wontfix label Jan 11, 2021
@alunyov
Copy link
Contributor

alunyov commented Jan 11, 2021

Thanks stale-bot. I think we still need the issue. I'm looking at converting the rest of the tests. It may take a while, but we'll see.

@poteto poteto added the pinned Pinned issues will never be marked stale by stalebot label Feb 10, 2021
@alunyov
Copy link
Contributor

alunyov commented Mar 15, 2021

Non-watchman file source. Currently we convert the directories in config.json into a watchman query for watch mode and simple builds. This was designed with non-watchman in mind and it should be possible to build out something that uses a different file-crawler and maybe file watcher crate. This is not strictly required, but I think removing this external required dependency would be nice. (Details in #3193)

One more point on why we may need to work on this: the current watchman client for rust doesn't work on Windows (I was testing it with github actions for windows: https://github.com/alunyov/relay/runs/2104656440?check_suite_focus=true). So we may add something like: https://github.com/notify-rs/notify in OSS.

@kassens kassens pinned this issue Mar 23, 2021
@kassens
Copy link
Member Author

kassens commented Mar 25, 2021

Baby steps, the example project todo app now compiles with the rust based compiler:

relayjs/relay-examples@470c497

NOTE: this is currently linux + mac only and requires watchman. Windows support is definitely a hard blocker for us, this is just an early preview to see where the problems are.

@maraisr
Copy link
Contributor

maraisr commented Sep 1, 2021

Can we also look at supporting "watch-mode" for OSS?

Currently we're forced to use hg??

.expect("Expect `hg` command getting base revision.");

@TrySound
Copy link
Contributor

TrySound commented Sep 1, 2021

Will rust compiler support es modules? Looks like it was not implemented. Would be good to have it from the beginning to avoid shipping separate features without support like happened with refetchables.
https://github.com/facebook/relay/blob/main/compiler/crates/relay-codegen/src/js_module_format.rs

@maraisr
Copy link
Contributor

maraisr commented Sep 1, 2021

@TrySound do you actually know where that js_module is actually used? Afaik it makes no difference? Or perhaps just with the langauge: "typescript" it isnt. I get import { ConcreteRequest } from 'relay-runtime'; in my __genearted__.

Or is that more about JSDepedency aka @module?

@alunyov
Copy link
Contributor

alunyov commented Sep 2, 2021

Can we also look at supporting "watch-mode" for OSS?

Currently we're forced to use hg??

.expect("Expect `hg` command getting base revision.");

Oh, yes, this is something that we need to fix. hg is definitely not a requirement for a watch mode.

@Brooooooklyn
Copy link

Brooooooklyn commented Sep 2, 2021

Friendly tips: If you have plans to expose Rust API to Node.js, you can take a look at napi-rs. For the pre-compile & testing & publish workflow, here is a demo: https://github.com/Brooooooklyn/snappy/releases/tag/v7.0.3

@mrtnzlml
Copy link
Contributor

Hi, I would like to propose to include the following issues/bugs into the list as well:

Thanks! 😎

@alunyov
Copy link
Contributor

alunyov commented Nov 2, 2021

hg issue fixed here: d336460

@huv1k
Copy link

huv1k commented Jan 3, 2022

There is a problem with @argumentDefinitions and defaultValue for @refetchable connections. Error is bellow:

    3 │       @argumentDefinitions(
    4 │         first: { type: "Int!", defaultValue: 2 }
      │                                              ^
    5 │         after: { type: "String" }

[ERROR] ✖︎ Non-nullable variable 'first' has a default value.

Query:

      fragment noteOwnerUsers on Query
      @argumentDefinitions(first: { type: "Int!" }, after: { type: "String" })
      @refetchable(queryName: "noteOwnersUsersConnection") {
        users(first: $first, after: $after)
          @connection(key: "noteOwnersUsersConnection_users") {
          edges {
            node {
              id
              email
            }
          }
        }
      }

@zth
Copy link
Contributor

zth commented Jan 5, 2022

@huv1k this is actually a proper validation that has started working with the new compiler. So, the solution is to go ahead and remove the default value, or make the argument optional by removing the ! from the definition.

@alunyov alunyov unpinned this issue Jan 14, 2022
@alunyov
Copy link
Contributor

alunyov commented Jan 14, 2022

v13 released. Closing this in favor of #3749

@alunyov alunyov closed this as completed Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned Pinned issues will never be marked stale by stalebot
Projects
None yet
Development

No branches or pull requests

9 participants