Add feature flag for fragment arguments

[tobias-tengler]

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.

[captbaritone]

Is this a meaningful change, or is the function just moving?

[tobias-tengler]

Just moving the function around. The grouping felt weird.

[facebook-github-bot]

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

[captbaritone]

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]

Sounds good, one sec :)

[tobias-tengler]

@captbaritone Done!

[facebook-github-bot]

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

[facebook-github-bot]

@captbaritone merged this pull request in d3c8d1c.

[tobias-tengler]

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 graphql/graphql-js#4015 :/
Fingers crossed it lands soon!

[tobias-tengler]

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]

@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 graphql/graphql-js#4015 landing in a stable enough release that Prettier can adopt it, and then also waiting on Prettier cutting some type of release.

[captbaritone]

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)