Can we make fragment reference related properties optional?

[alloy]

For those that test their Relay containers by manually stubbing data, it would be nice to have the fragment reference related properties be optional so that those don’t need to be specified.

I don’t recall the exact mechanics of the future fragment reference support when TS 2.8 comes out, but would an alternative option be to somehow use a union, so that either the exact data or a correct fragment reference can be passed in?

const enum foo$ref { }

interface Props {
	bar: string
}

function foo(x: Props | { " $refType": foo$ref }) { }

foo({ bar: "bar" })
foo({ " $refType": ({} as any) as foo$ref })

[kastermester]

Can you provide some examples of tests where this would be used? I'm not entirely clear on the semantics we're trying to type up here. But it is definitely a good idea to support testing better! :)

[alloy]

Something like this:

const MyComponent = createFragmentContainer(
  props => <div>{props.artist.name}</div>,
  graphql`
    fragment MyComponent_artist on Artist {
      name
    }
  `
)

it("renders the artist’s name", () => {
  const artist = { name: "banksy" }
  const wrapper = shallow(<MyComponent artist={artist} />)
  expect(wrapper.find('div').text()).toEqual("banksy")
})

[kastermester]

And this actually works? I didn't think that was allowed? Does Relay generate warnings for this?

[alloy]

You need to mock the Relay API, such that it doesn’t actually do much of anything https://github.com/artsy/emission/blob/master/__mocks__/react-relay.js, but then it works yeah.

It also works without mocking, but then Relay will indeed give a warning facebook/relay#2291.

We’re actually planning on moving to a stubbed local schema in the near future, but we’re definitely not the only ones doing this, so just something to keep in mind.

[kastermester]

This is definitely something to think about. I'm kinda torn up on this. Given that I'd not want to miss out on production errors that could occur such as this:

const Parent = createFragmentContainer(ParentComponent, graphql`fragment Parent_data on Todo { id text }`);

const Child = createFragmentContainer(ChildComponent, graphql`fragment Child_data on Todo { text }`);

Given that the Parent renders the Child - if we change the type definitions (to what you proposed) then no type errors will happen here, but still warnings when running the code.

Really what we'd like would be some sort of way to have different type definitions loaded under tests compared to when the actual production code is written. If people are dumping all their tests under one __tests__ folder, then this could be accomplished by simply putting a different tsconfig in that folder, redirecting react-relay (and friends) to files which strip out the special fragment tainting that we do in the compiler.

[alloy]

Agreed, it’s a slippery slope.

Actually I like the idea of having different types for tests, it could also disable Relay’s masking feature and thus provide types that actually help the dev to know all of the data they need to provide for the tree of the component they are working with.

What are you doing with tests, a local stubbed copy of your schema?

[kastermester]

We're still in the process of getting our codebase under test coverage - so far the focus area has been all the server code, so we don't really have that much experience in testing the client code, sadly.

I think testing an in memory copy of the schema is quite a nice way of dealing with it though. But certainly also a much more involved method (it would be nice to be able to test just a single component I think).

If we want to support the idea of having different types to require for tests, the next question is where to put the "mock" type definition files? Do they go into definitely typed? Or this repo? One would more or less have to change the tsconfig under compilerOptions to be something like:

{
  "compilerOptions": {
    "paths": {
      "react-relay": ["somepathhere"]
    }
  }
}

[alloy]

I guess that depends on wether or not the user needs to use this plugin for that or not. If it’s not necessarily a hard requirement, then it probably makes more sense to add it to DT.

[alloy]

Just so you know, I’m going to be focussing more on the move to local stubbed schema for now, so maybe it makes most sense to continue this when either you get to wanting such tests or I decide we still need to be able to do this.

[kastermester]

Agreed, the only thing is that it would mean adding some valid module path that doesn't actually exist - I don't know what the policy on doing that is inside DT.

Just so you know, I’m going to be focussing more on the move to local stubbed schema for now, so maybe it makes most sense to continue this when either you get to wanting such tests or I decide we still need to be able to do this.

Sounds like a good idea. It is worth remembering though :) But at least this should be something that people can experiment with, without needing our interaction.

[alloy]

I think that as long as we can get it to work in DT it’ll be fine.

Totally! 👍

[cvle]

We have some tests that stubs the props data of e.g. Fragment Containers. The types were failing since I upgraded because of $fragmentRefs and $refType which weren't there before.

My current solution is to remove those just for the specific tests that requires it.

Here is how we do it, maybe this is helpful for you too.

/** Get the Prop Type of a Component */
type PropTypesOf<T> = T extends React.ComponentType<infer R>
  ? R
  : T extends React.Component<infer S> ? S : {};

/** Remove all traces of `$fragmentRefs` and `$refType` from type recursively */
type NoFragmentRefs<T> = T extends object
  ? {
      [P in Exclude<keyof T, " $fragmentRefs" | " $refType">]: NoFragmentRefs<T[P]>
    }
  : T;

/** Remove fragment refs from a Component Type */
type NoFragmentRefsComponent<T> = ComponentType<
  NoFragmentRefs<PropTypesOf<T>>
>;

// Remove fragment refs so we can stub the props.
const CommentContainerN = CommentContainer as NoFragmentRefsComponent<
  typeof CommentContainer
>;

it("renders correctly", () => {
  const props: PropTypesOf<typeof CommentContainerN> = {
    data: {
      id: "comment-id",
      author: {
        username: "Marvin",
      },
      body: "Woof",
      createdAt: "1995-12-17T03:24:00.000Z",
    },
  };

  const wrapper = shallow(<CommentContainerN {...props} />);
  expect(wrapper).toMatchSnapshot();
});

[alloy]

That’s great, thanks @cvle!

[kastermester]

@cvle that's a pretty sweet solution :)

It would be kinda sweet to come up with an easier way to do this though, but something like your solution should work while we still think about that :)

[cvle]

Agree. I'm a bit sceptical with using a different set of types for tests though, it sounds like a configuration nightmare.

Btw I simplified the above solution a little bit:

/** Remove all traces of `$fragmentRefs` and `$refType` from type recursively */
type NoFragmentRefs<T> = T extends object
  ? {
      [P in Exclude<keyof T, " $fragmentRefs" | " $refType">]: NoFragmentRefs<
        T[P]
      >
    }
  : T;

/** Remove fragment refs from Proptypes so we can stub the props. */
function removeFragmentRefs<T>(
  component: ComponentType<T>
): ComponentType<NoFragmentRefs<T>> {
  return component as any;
}

const CommentContainerN = removeFragmentRefs(CommentContainer);

[kastermester]

I agree - however the point is that the types are different (runtime and compile time) - as the types encode the idea that Relay will be able to find the data needed for any containers rendered using the data.

This is definitely an area where some helper code would come in handy.

Have you considered flipping the problem on its head? I'm thinking something like:

type WithFragmentRefs<T> = T extends object ? { [P in keyof T]: WithFragmentRefs<T[P]> } & { " $fragmentRefs": any; " $refType": any } : T;

function withFragmentRefs<TProps>(props: TProps): WithFragmentRefs<T> {
  return props as any;
}

Now you should be able to take any plain props and pass them through the function which should add the needed "magic" types to the props such that you won't get type errors. (warning: I have not tested the above code)

[cvle]

Not sure if I can follow your idea, we use the generated types in our props so they already include the fragmentref related props. Even if we could have the plain props somewhere, I would still need some typescript casting magic to convince it to use a component with these props. I think for now removing the fragmentref related props manually in our tests works good enough.

[kastermester]

Well my solution does pretty much the inverse of yours, instead of removing the types from the React component I add them to the props such that you can do:

const props = withFragmentRefs({
  viewer: {
    id: 'my-id',
    name: 'Some name',
  },
});

const wrapper = <SomeContainer {...props} />;

expect(wrapper).toMatchSnapshot();

[alloy]

I’ve raised this issue, with the intent that you could have a dummy fragment in your tests that would generate the typings for the data needed by all components in the tree.

import { MyTest_fullTypings } from "__generated__/MyTest_dummy.graphql"

graphql`
  fragment MyTest_fullTypings on Foo {
    ...TestSubjectComponent_foo @relay(maskRecursively: false)
  }
`

const mockData: MyTest_fullTypings = {
  // ...
}

[alloy]

I’m personally looking into using an actual mocked copy of the schema to run tests against, thus ensuring everything around Relay is actually wired up correctly.

[cvle]

@kastermester gotcha! That's interesting. Wondering how a proper withFragmentRefs implementation would look like 😄

@alloy also interesting, we are using https://github.com/relay-tools/relay-local-schema to mock out the schema. Currently the data is without types, I was thinking of using https://github.com/dangcuuson/graphql-schema-typescript maybe as we do successfully on the server side, but your idea would also be interesting because the typings are narrowed down.