relay

relay copied to clipboard

This is another attempt at https://github.com/facebook/relay/pull/4421. I've removed the automatic renaming part to make the feature purely opt-in (can still be made automatic later, maybe also behind a flag).

Changes

• Adds support for the following rename operations
  • Renaming operations
  • Renaming fragments (both from definition and spread)
  • Rename fragment arguments (both from definition and usage)
  • Rename operation variables (both from definition and usage)
    • Global variables aren't yet supported. I plan to add support for this in a later PR alongside better LSP support for global variables in general

Thanks for continuing to work on this!

1. Can "Fix "goto references" for TerseRelayResolver" be split into a separate PR?
2. How could we add unit tests for this code? Happy to hop on a VC if you'd like and we can brainstorm

@captbaritone Dammit, missed the release window again 😅 Would still love to get this in though!

@captbaritone to your above points:

1. Yes, not supporting global variables is intentional (for now).
2. If you can provide me with an example of a project that inherits fragments from a base project, I'll gladly test it. I wasn't able to set it up, as reported in https://github.com/facebook/relay/issues/4471 If you can find the fragment references in a child project via the LSP's "find references" from the parent project, renaming them will also work, as I'm using the same mechanism for renaming references.
3. I have pulled the refactor out of this pull request: https://github.com/facebook/relay/issues/4600
4. I've created a playground for these changes here: https://github.com/tobias-tengler/relay-lsp-playground. Here are some videos showcasing the new functionality end-to-end:

Renaming operation

https://github.com/facebook/relay/assets/45513122/ad962887-e741-446f-a4a2-e7127d045bf5

Renaming fragment (from definition)

https://github.com/facebook/relay/assets/45513122/197d7abe-bb0f-4dce-86e8-647ecb2fadbc

Renaming fragment (from spread)

https://github.com/facebook/relay/assets/45513122/f158d0d3-c817-4030-80a4-77200ad67769

Renaming variable (from definition)

https://github.com/facebook/relay/assets/45513122/426c6a7c-10b6-42a7-b380-5aee84cfdf63

Renaming variable (from usage)

https://github.com/facebook/relay/assets/45513122/1e2aa315-dc1d-4134-a092-c397fc279a7a

Rename fragment argument (from definition)

https://github.com/facebook/relay/assets/45513122/0eec04b1-fb19-493b-889b-59df212f7e14

Rename fragment argument (from usage)

https://github.com/facebook/relay/assets/45513122/119416be-d5c0-45fd-a81d-4c349ba094ab

@captbaritone I've updated this to also support renaming of proper fragment arguments (variable definitions) now that #4651 is merged.

Would love to see this merged. It pains me that this is dragging around since last August.

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

I've been able to get this working internally, which is rad!

However, I noticed one case which does not seem to work correctly:

const UserProfile = graphql`
  fragment RelayReaderTestCreatesFragmentPointersWithVariableArgumentsUserProfile on User
  @argumentDefinitions(size: {type: "[Int]"}) {
    id
    ...RelayReaderTestCreatesFragmentPointersWithVariableArgumentsUserProfilePicture
      @arguments(size: $size)
  }
`;

When I try to rename the $size variable it only renames the definition, not it's value in the @arguments directive.

The fragment I was testing on: https://github.com/facebook/relay/blob/c8c09f94f6018eb967492bec770e9892243d8b1d/packages/relay-runtime/store/tests/RelayReader-test.js#L286

https://github.com/facebook/relay/assets/162735/637f4a8c-69d4-4f13-9c8d-439e22e56ab1

I'm able to reproduce this with a test. Working on a fix :)

@captbaritone Fixed!

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

FYI the LSP crate has some other internal users which I will need to update to work with these changes. It's on my todo list for next week.

@captbaritone Also fixed the conflicts here. Ready to be merged again :)

@captbaritone This would also be awesome to have in today's release 🤞

@captbaritone Were you able to address the issue with the internal users of the LSP crate? Is there anything I can do to get this merged?

@captbaritone The one year anniversary of me first raising a PR for this is coming up, so would you do me the honors and get this through? 😇 If you can give me some hints what exactly is blocking internally, I could maybe come up with a solution that's easier to land as well!

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

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

@captbaritone Tysm ❤️ Much appreciated!

Sorry for the egregious delay. Still need to get our GitHub CI building reliably in order to get @main builds publishing to NPM. Currently it's hitting issues running out of disk space.