search

facebook / relay

Relay is a JavaScript framework for building data-driven React applications.
https://relay.dev
MIT License
18.27k stars 1.8k forks source link

Add feature flag for fragment arguments #4648

Closed tobias-tengler closed 3 months ago

tobias-tengler commented 3 months ago

Adds a new enable_fragment_argument_transform feature flag that allows to enable the existing parsing and transform of fragment variable definitions and fragment spread arguments.

facebook-github-bot commented 3 months ago

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

captbaritone commented 3 months ago

What do you think about adding an integration test fixture here: https://github.com/facebook/relay/tree/c2d0901e0ac535e36f94fa23b1e4dfc9324e3bc7/compiler/crates/relay-compiler/tests/relay_compiler_integration/fixtures?

Could validate that with the flag enabled we correctly parse the new syntax

tobias-tengler commented 3 months ago

Sounds good, one sec :)

tobias-tengler commented 3 months ago

@captbaritone Done!

facebook-github-bot commented 3 months ago

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

facebook-github-bot commented 3 months ago

@captbaritone merged this pull request in facebook/relay@d3c8d1c2330013d178c159f4418407ed41528283.

tobias-tengler commented 3 months ago

Thanks for merging :) I just wanted to play around with it in our real codebase and noticed that we're still blocked by eslint-plugin-relay's graphql-syntax rule until we get https://github.com/graphql/graphql-js/pull/4015 :/ Fingers crossed it lands soon!

tobias-tengler commented 3 months ago

Actually I'm not really sure why we need that rule in the first place. Seems like it could be fully replaced by the compiler, if the compiler were to ensure that only one definition exists within each graphql-tagged-literal. Or is there another reason why we'd need the Lint Rule as well?

captbaritone commented 3 months ago

@tobias-tengler I looked into this same thing a bit yesterday from an internal perspective: "What would it take to enable this internally".

  1. Lint rule - The syntax check fails immediately. As you point out the compiler checks this, eventually, so in the short term you could disable the rule. However, parsing is used by all the other rules as well, but they likely ignore syntax errors. So I think it's good to keep the syntax rule long term. Otherwise a syntax error just silently suppresses all lint errors which feels unexpected.
  2. Prettier - This one feels like a hard blocker. Prettier has support for the deprecated syntax enabled, but I think that only applies to argument definitions, not argument passing.
  3. Syntax Highlighting - It looks like this has been supported for over a year by the VSCode extension we pull in as a dependency of Relay's extension, however I need to double check if Flow's VSCode extension is still shipping their own GraphQL syntax highlighting.

I think Prettier is the big blocker here. We have to have auto-format working and that will depend upon https://github.com/graphql/graphql-js/pull/4015 landing in a stable enough release that Prettier can adopt it, and then also waiting on Prettier cutting some type of release.

captbaritone commented 3 months ago

Update: I looked into the syntax highlighting issue for internal use at Meta, and Flow still depends upon https://github.com/michaelgmcd/vscode-language-babel/ for syntax highlighting and it defines its own GraphQL syntax highlighting still. I think it should probably just opt to pull in https://marketplace.visualstudio.com/items?itemName=GraphQL.vscode-graphql-syntax as a dependency instead (as we do with Relay's own VSCode extension)