[RFC]Best practices on exporting/importing relay containers

[jo-rs]

Proposal:

Establish a standard for naming, importing and exporting components and/or relay containers.

One way to go about this would be to always be explicit about what is being exported, and if aliasing must happen always be more descriptive, not less. So, MyThing would be a React component, MyThingFragmentContainer would be a relay container, and so on. And, if MyThing has an associated interface named MyThing, it should be aliased as MyThingType, if necessary.

However, this isn't Java, and some of these would get quite cumbersome so any suggestion on what a less painful/inconvenient way of avoiding confusion is welcome.

Reasoning

Recently there have been at least three separate instances of issues, that I'm aware of, caused by a lack of protocol in regards to exporting/importing relay fragments.

• A component file has two exports with the same name; a regular component, following the MyThing nomenclature, and a default relay FragmentContainer.

export const MyThing: React.FC<Props> = ...

export default createFragmentContainer(MyThing, ...

Leading to an already existing import statement, which would be expected to look something like import MyThing from '...', to actually look like import { MyThing } from '...'.

• A fragment container is aliased on import

import { MyThingFragmentContainer as MyThing } from '...'

---

The problem being that it causes confusion, and leads to situations where you're expecting to be working with a regular component, but it's actually a relay container, or vice-versa. Because of the nature of the error statements resulting from this it can be frustrating to debug, unless you know exactly what to look for.

Exceptions:

None?

How is this RFC resolved?

We agree on a standardised approach to naming relay containers.

Done in partnership with @admbtlr.

[mzikherman]

Interesting! My initial take is we don't need to standardize this, since with TypeScript, VSCode, etc. it should be 'easy' to get the right import.

Personally I organize my component file something like:

interface Props -> I name the React component props, the imaginative name of...Props 😅 . I also don't export this as it's usually not needed elsewhere. Prefacing it with the module name feels redundant. Of course it's the props for this component! What other Props would they be?

[export] const ArtistCard -> I name the vanilla React component a 'plain' name, and I only optionally export it. We'll almost always be using the Relay-wrapped component, and so why even export this? Unless you're using the vanilla React component (storybook which is deprecated, or in tests, in which one should be testing the Relay container anyway esp. with the new Relay testing utils).

export const ArtistCard[Fragment|Refetch|Pagination]Container -> This is the Relay-wrapped container, which does almost always need to be exported. Due to the React component of the same name living in the file, I usually name with with the Relay nomenclature. However, I usually import it as: import {ArtistCardFragmentContainer as ArtistCard}, since in the calling code, it's redundant to include the Relay-specific naming.

I generally skip default exports. It's my understanding those are not as desirable as explicitly named ones, and it's only some limited set of tooling that may require a default export (generally of the Relay container). If so, I would add that only if needed, and with a comment.

// Default export needed for analytics integration.
export default ArtistCardFragmentContainer

Now, this is just my convention! I suspect most people probably have a similar personal convention they've adopted. Overall I don't think this kind of convention is a big lift (and seems like it can be enforced via a Peril rule), but I guess I'm not seeing the issues that come from differing naming conventions around (ie- it should be pretty straightforward to get the right import).

[ashfurrow]

I'm a 👍 to standardizing – the conventions that Matt outlines are generally how I work as well, except I don't rename during imports. Maybe it's the Objective-C programmer in me, but I'll take a more precise, explicit name over a more concise, ambiguous name any day.

As I've taught more and more engineers how Relay works in Eigen, the inconsistencies really add up to make it more difficult than it has to be. I'm also 👍 to avoiding default exports, which we've been avoiding on Eigen for some time.

[sepans]

We'll almost always be using the Relay-wrapped component, and so why even export [vanilla component]?

Most of the time I have seen 👆 was to find the component by type in the tests. If we address/standardize that I don't think we really need to export vanilla component and it mostly addresses the pain point of this RFC. I am still 👍 for a standard because it makes reading and searching the code easier.

Renaming:
I like renaming because it makes the parent component less verbose and it sort of encapsulates the underlying relay part of the child component but either way I am ok with that.

[zephraph]

I think the renaming/aliasing part is one of the bigger confusing parts. I know @admbtlr ran across this, so maybe he can speak to it more. The issue is if you pass the react component but you meant to pass the fragement container you'll get pretty mysterious errors. That's something we could lose a lot of time to. Just removing the alias would help solve for that.

[damassi]

On Eigen we've been gradually removing aliasing from relay stuff, so this would bring things in line, although personally I do think it's a bit overkill since VSCode will resolve aliases as expected. (Although although, when I was removing My Collections aliases I noticed that it cleaned up some other stuff quite nicely (related to jest mocking) so I've just resolved to myself to it being better for our code while not being too much of a hinderance readability-wise.)

Regarding exports of non relay code for tests, etc., the pattern I've been using for a while tries to preserve the distinction between public and private by explicitly exporting stuff into a tests object that is then imported in to the test. For example:

const Foo = props => {
  return <div>{props.artist.artistName}</div>
}

export const FooFragmentContainer = createFragmentContainer(Foo, {
  artist: graphql`
     ...
`})

export const tests = {
  Foo
}

It might be overkill, but having a lot of exports makes code feel somewhat indirect and confusing to read / maintain through time, and tests should still be able to get at "private" code easily -- hence, this compromise.

Lastly: Matt mentions it above, but yeah, unless a component is bundle split via a dynamic import statement, we should always avoid default exports because default exports aren't statically analyzable -- meaning, a default export can then be imported as anything, leading to code drift over time. A named export can be imported as one thing and one thing only (unless it's explicitly aliased, which here we're trying to avoid).

[MounirDhahri]

I am also 👍.

I followed the same patterns Matt described (including aliasing the imports) and it helped me immensely. However, recently it has been problematic since VSCode imports the standard file (if it's exported for whatever reason) and I end up spending time figuring out why typescript checks are failing to find out in the end that I’m not using the [Fragment|Refetch|Pagination]Container. Also while reviewing some PRs, I noticed a few times the same mistake and workarounds to fix the typescript errors or using the standard component for the tests without mocking the relay containers.

Standardizing exports/imports containers may add up more restrictions to developers in the beginning but as Chris said, "it will help us avoid indirect and confusing to read/maintain code through time".

[pvinis]

I don't mind either way, but I will just throw my opinions here in case they are at all useful.

• I don't like using default exports, mostly to keep everything consistent with the { Foo } import style, and because tsserver or vscode or whatever doesn't know to autoimport because it has no name. And if it has a name, it's shorter to just export inline than having a new line just to default export a named thing.

• I don't like renaming imports. It messes with my brain. When I see Foo but it's actually FooContainer renamed, and there is an actual Foo thing, my brain thinks Foo is Foo, not FooContainer. Can you blame it? Because of that, I often prefer to name things like:

• FooComponent

• Foo
  instead of:

• Foo

• FooContainer
  This way the "external"/exported thing is named something that makes sense, and then "internal" thing that I don't need to often import/use can be named something "internal" like FooComponent, rather than having the inverse and needing renaming.

[jo-rs]

This doesn't seem to have been too controversial, so I'm going to assume it's been accepted 🤔

Takeaways:

• Don't use default exports
• Don't alias imports
• Use explicit naming

Let's start with this. @admbtlr suggested that we could look into implementing a peril rule eventually; potentially as a FF project.

I'll carve out some time to check out on some instances of this in the codebase.

Thank you, everyone who took the time to read through, and share your thoughts!